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

Addon-info: deep merge passed styles with default ones #2449

Merged
merged 12 commits into from
Dec 18, 2017

Conversation

jennvoss
Copy link
Contributor

@jennvoss jennvoss commented Dec 8, 2017

Issue:
The styles option for addon-info was improperly documented
#2144

What I did

Updated addon-info docs to include the proper reference for the styles option and updated the example in cra-kitchen-sink

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jennvoss

@Hypnosphi
Copy link
Member

Hypnosphi commented Dec 8, 2017

Please run yarn test --core --update to update snapshots of our example stories. You may need to bootstrap the project with yarn bootstrap --core first if you didn't yet

@jennvoss
Copy link
Contributor Author

jennvoss commented Dec 8, 2017

thanks @Hypnosphi, i will do that.

Also found a deeper issue with this - if you don't include properties that contain nested objects from the stylesheet object (https://github.com/storybooks/storybook/blob/master/addons/info/src/components/Story.js#L19) such as "button", "header" , or "source" then the storybook will throw an error. Will try to look into this more over the weekend.

@codecov
Copy link

codecov bot commented Dec 10, 2017

Codecov Report

Merging #2449 into release/3.3 will increase coverage by <.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #2449      +/-   ##
===============================================
+ Coverage        19.51%   19.51%   +<.01%     
===============================================
  Files              386      386              
  Lines             8707     8706       -1     
  Branches           942      954      +12     
===============================================
  Hits              1699     1699              
+ Misses            6276     6255      -21     
- Partials           732      752      +20
Impacted Files Coverage Δ
addons/info/src/components/Story.js 31.4% <ø> (+0.15%) ⬆️
app/react/src/client/preview/render.js 0% <ø> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
addons/info/src/index.js 78.57% <100%> (ø) ⬆️
addons/info/src/components/types/ArrayOf.js 14.28% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/search_box.js 36.36% <0%> (ø) ⬆️
.../ui/components/stories_panel/stories_tree/index.js 41.98% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Elements.js 0% <0%> (ø) ⬆️
addons/actions/src/lib/types/object/index.js 25% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bf9701...5efa669. Read the comment docs.

@jennvoss
Copy link
Contributor Author

Update: I changed the styles prop to accept an object instead of a function, and used nested-object-assign to only overwrite the properties that were passed. Hot reloading still works fine this way (#2144 mentioned a concern about this). Updated the README, cra-kitchen-sink example and snapshots.

@@ -382,7 +383,7 @@ Story.propTypes = {
showInline: PropTypes.bool,
showHeader: PropTypes.bool,
showSource: PropTypes.bool,
styles: PropTypes.func.isRequired,
styles: PropTypes.object, // eslint-disable-line react/forbid-prop-types
Copy link
Member

Choose a reason for hiding this comment

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

Let's rather make it PropTypes.oneOfType(PropTypes.func, PropTypes.object) and add a runtime check. If user supplies a function we call it, otherwise do a deep merge.

That way we won't introduce any breaking changes, all the old code will keep working

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, will update

Copy link
Member

@Hypnosphi Hypnosphi Dec 13, 2017

Choose a reason for hiding this comment

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

Oh, we actually do that runtime check already:
https://github.com/storybooks/storybook/blob/master/addons/info/src/index.js#L56

Let's just replace the default s => s function with deep merge (s => nestedObjectAssign({}, s, styles))

@Hypnosphi Hypnosphi changed the base branch from master to release/3.3 December 13, 2017 16:19
@Hypnosphi
Copy link
Member

I changed the base of this PR to release/3.3 because now it introduces a new feature. If you need any help with conflicts resolution etc., feel free to contact us on slack: https://now-examples-slackin-nqnzoygycp.now.sh/

@Hypnosphi Hypnosphi changed the title Update addon info docs Addon-info: allow passing styles as objects Dec 13, 2017
@Hypnosphi Hypnosphi changed the title Addon-info: allow passing styles as objects Addon-info: deep merge passed styles with default ones Dec 13, 2017
@Hypnosphi
Copy link
Member

Styles option as function is still worth documenting, and the example should indicate that default stylesheet is passed as argument. It can look like that:

styles: stylesheet => ({
  ...stylesheet,
  header: {
    ...stylesheet.header,
    h1: {
      ...stylesheet.header.h1,
      color: 'red'
    }
  }
})

This is more verbose than just passing an object that would be deep merged, but at the same time gives more control. This is kinda like Extend vs Full Control mode for custom webpack config: https://storybook.js.org/configurations/custom-webpack-config/

@Hypnosphi Hypnosphi self-assigned this Dec 13, 2017
Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

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

looks good, just add the style function backwards compatibility and we're good to go :)

@jennvoss jennvoss force-pushed the update-addon-info-docs branch from 7a49de8 to bd83798 Compare December 17, 2017 17:57
@@ -384,7 +384,7 @@ Story.propTypes = {
showInline: PropTypes.bool,
showHeader: PropTypes.bool,
showSource: PropTypes.bool,
styles: PropTypes.func.isRequired,
styles: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this proptype may be left as is (we actually pass a function anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done

@Hypnosphi Hypnosphi added this to the v3.3.0 milestone Dec 18, 2017
@Hypnosphi
Copy link
Member

Please update the storyshots one more time

@jennvoss
Copy link
Contributor Author

When i run yarn test --core --update, everything passes and no updates are present. Little lost as to why it fails here. I'll make another change to the cra-kitchen-sink example and try again.

@jennvoss
Copy link
Contributor Author

Looks like i needed to run yarn bootstrap --core again as well

@Hypnosphi Hypnosphi merged commit cab9f4d into storybookjs:release/3.3 Dec 18, 2017
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