Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Move theme registration to src #182

Merged
merged 2 commits into from
Jul 17, 2018

Conversation

philipfweiss
Copy link
Contributor

@philipfweiss philipfweiss commented Jul 16, 2018

Addresses #181. Moves the theme registration helper functions into src, so they will be compiled during the build step. Also updates the callsites of these functions to point into lib/utils/<x>, as desired.

Keeps a copy of these files in scripts/utils as well, for backwards-compatibility. These files should be removed at the next major version update.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is either semver-minor (adding new things) or semver-major (existing consumers of the scripts dir will break).

@philipfweiss
Copy link
Contributor Author

philipfweiss commented Jul 16, 2018

@ljharb I believe public interface being exposed hasn't changed, so this should be semver-minor.

@ljharb
Copy link
Collaborator

ljharb commented Jul 16, 2018

@philipfweiss all accessible files are part of the public interface.

@philipfweiss philipfweiss force-pushed the pw--move-theme-registration-to-src branch from ab58b62 to e2a999a Compare July 16, 2018 21:37
@philipfweiss philipfweiss force-pushed the pw--move-theme-registration-to-src branch from e2a999a to 64a665a Compare July 16, 2018 21:42
@philipfweiss
Copy link
Contributor Author

philipfweiss commented Jul 16, 2018

@ljharb Ok! Based on a discussion with @majapw, we will keep a copy of the files in scripts/utils, which should be removed at the next major version update.

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments! :)

@@ -2,7 +2,7 @@ import { configure } from '@kadira/storybook';
import ThemedStyleSheet from 'react-with-styles/lib/ThemedStyleSheet';
import aphroditeInterface from 'react-with-styles-interface-aphrodite';
import '../css/rheostat.css';
import registerCSSInterfaceWithDefaultTheme from '../scripts/utils/registerCSSInterfaceWithDefaultTheme';
import registerCSSInterfaceWithDefaultTheme from '../lib/utils/registerCSSInterfaceWithDefaultTheme';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this update. Let's pull from src here.

@@ -1,7 +1,7 @@
import { configure } from '@kadira/storybook';
import ThemedStyleSheet from 'react-with-styles/lib/ThemedStyleSheet';
import aphroditeInterface from 'react-with-styles-interface-aphrodite';
import registerInterfaceWithDefaultTheme from '../scripts/utils/registerInterfaceWithDefaultTheme';
import registerInterfaceWithDefaultTheme from '../lib/utils/registerInterfaceWithDefaultTheme';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this update. Let's pull from src here. Otherwise we'll have to build all the js every time we run storybook

@@ -0,0 +1,7 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, I don't think we want this eslintrc here. the src/utils folder should not be importing dev dependencies.

@@ -1,3 +1,5 @@
/* This file should be removed in the next major version update of Rheostat */

This comment was marked as resolved.

@@ -1,3 +1,5 @@
/* This file should be removed in the next major version update of Rheostat */

This comment was marked as resolved.

@majapw
Copy link
Collaborator

majapw commented Jul 17, 2018

Looks great to me!

@majapw majapw merged commit d1c2582 into master Jul 17, 2018
@majapw majapw deleted the pw--move-theme-registration-to-src branch July 17, 2018 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants