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-backgrounds: Add docs support and extend grid configuration #12368

Merged
merged 17 commits into from Sep 25, 2020

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Sep 3, 2020

Issues: #7978 #11128 #8575 #12147 and potentially #12356

What I did

Context: as soon as users add theme into their storybook, which usually applies a background color to the body of the canvas element, addon backgrounds along with grid would become useless, given that the css from the theme was more specific than the one set in the iframe from the addon.

This PR introduces the following changes:

Fixes

  • Allow backgrounds and grid to work alongside themes that set a background color to the body element.

backgrounds-gif

  • Grid adds a default 16px offset when stories have a padded layout so it aligns correctly with the components:

image

  • The offset is zero when in fullscreen layout:

image

  • The grid scales together when story is zoomed.

image

Features

  • Enable backgrounds and grid in docs view. This seems to bring extra value for chromatic as well 👌
    *Unfortunately grid doesn't scale when the story is zoomed in docs.

backgrounds-docs

  • Add more customization to the grid, as well as possibility to disable it independently of backgrounds. Here are the default values:
Button.parameters: {
  backgrounds: {
    grid: {
      disable: false,
      cellSize: 20,
      opacity: 0.5,
      cellAmount: 5,
    }
  }
}

Extra changes

In this PR there are a few things which are somewhat related and I took the liberty to change as well:

  • Add viewMode to the context object that is passed in decorators
  • Do a deep equal check when updating globals and only update when the objects are different
  • Apply minimum height of 100vh to the docs wrapper to make sure it takes full height. Before it would just take the height that it fill, but if people have a theme that sets color to the body, that color would be shown in docs in the remaining space at the bottom.
  • Add two examples to the official storybook using backgrounds with gradient and image:
    image

How to test

1 - Checkout the repo, bootstrap it
2.1 - Either run E2E with yarn test:e2e-framework react --use-local-sb-cli
2.2 - yarn start and test it yourself

  • Is this testable with Jest or Chromatic screenshots?
    Yes, I also added some E2E tests for this

  • Does this need a new example in the kitchen sink apps?
    I've added to the official-storybook example (which is used in e2e). If necessary I can also add to the others.

  • Does this need an update to the documentation?
    Yes, which I have done so, also in migration.

@yannbf yannbf marked this pull request as draft September 3, 2020 09:23
@yannbf yannbf self-assigned this Sep 8, 2020
@yannbf yannbf marked this pull request as ready for review September 8, 2020 12:09
@yannbf yannbf changed the title WIP: revamp addon backgrounds Addon-backgrounds: add docs support and extend grid configuration Sep 8, 2020
@shilman shilman changed the title Addon-backgrounds: add docs support and extend grid configuration Addon-backgrounds: Add docs support and extend grid configuration Sep 10, 2020
@shilman shilman added this to the 6.1 essentials milestone Sep 10, 2020
@shilman
Copy link
Member

shilman commented Sep 10, 2020

@yannbf amazing PR! 🙌 any idea what's going on with CI?

@yannbf
Copy link
Member Author

yannbf commented Sep 10, 2020

@yannbf amazing PR! 🙌 any idea what's going on with CI?

Yes, I need to update a few things related to the api given that there's a ViewMode in the story context type and I forgot to do that 🤦‍♀️ I'll do it soon!

@yannbf yannbf force-pushed the feat/revamp-addon-backgrounds branch 2 times, most recently from c4f6234 to bb6cc05 Compare September 13, 2020 14:16
@@ -406,6 +406,7 @@ export default class StoryStore {
args: _stories[id].args,
argTypes,
globals: this._globals,
viewMode: this._selection?.viewMode,
Copy link
Member Author

@yannbf yannbf Sep 13, 2020

Choose a reason for hiding this comment

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

I might be overlooking possible scenarios here.
@ndelangen @shilman could you please give a small hand here? I'm not sure if it's ok to just get the value from this._selection here. It doesn't seem to be available at all times when this runs. Should it be?

@shilman
Copy link
Member

shilman commented Sep 19, 2020

@yannbf yannbf force-pushed the feat/revamp-addon-backgrounds branch from 68063c7 to 52211de Compare September 20, 2020 14:31
@yannbf
Copy link
Member Author

yannbf commented Sep 22, 2020

@yannbf what's going on with the DocsPage diffs e.g.

https://www.chromatic.com/snapshot?appId=5a375b97f4b14f0020b0cda3&id=5f6273905740f4002245a96b

Hey @shilman I made two changes in docs elements:
1 - added a "docs-story" class so that the element can be selected to apply the background
2 - added minHeight of 100vh so that the docs page always take full height.

The reason for number 2 is that the docs page never took full height but we didn't notice because the background of the iframe was white, but if there's any rule that sets the background to the iframe or the body, then you will have results like this:
image

As this PR keeps the style in the body so that when you switch from docs to canvas, the background is already there and there's no flashing, by not having the docs page take full height you can have that result.

But anyway we can check if this is the proper way to fix this and I can rework on whatever is necessary!

@yannbf yannbf force-pushed the feat/revamp-addon-backgrounds branch from 52211de to e96f480 Compare September 22, 2020 09:51
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Finally had a chance to review this PR, and it's really great work. 👏 👏 👏

I think there are three sets of changes here:

  • A bunch of different backgrounds changes
  • Making viewMode available in the StoryContext
  • The e2e test-clearing code

Ideally this would be three different PRs, or even more because there are so many background changes.

I think splitting out the test-clearing stuff is not necessary, so I'd only like you to split out the viewMode change into its own PR so:

  • It can be easily referenced later if there are problems later
  • It can be marked as a feature not a bug

Aside from that I think it's ready to merge unless anybody else has objections. Thanks for your patience on the review!

- Previously it was applied to the iframe, and sometimes it became unusable
- given that in a few scenarios the background of the body had more specificity.
@yannbf yannbf force-pushed the feat/revamp-addon-backgrounds branch from e96f480 to 6c1bb7c Compare September 24, 2020 13:01
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

@sam-dassana
Copy link

Is there documentation on how to change the canvas background color with a toolbar theme control button?

I've looked at the theming docs, browsed through some of the github issues without much luck.

Do you have to write a custom hook like how this stackoverflow answer does?

I'm talking about just this part:
Sep-30-2020 13-38-47

@tmeasday
Copy link
Member

tmeasday commented Oct 1, 2020

@jonniebigodes ☝️ I've seen @sam-m-m's request here a couple times so if we could figure out the current best practice we should document it (also thinking about a way of supporting it "natively" without breaking the global's model too much).

@jonniebigodes
Copy link
Contributor

@tmeasday and @sam-m-m i've been thinking and this could be a prime candidate for a recipes page. I'm still mapping out some details and we'll need to take a closer look at the the implementation and how to best document it. Let me know what you think and we'll proceed from there.

@sam-dassana
Copy link

@jonniebigodes A recipe and native support both sound like great ideas. Allowing options to change theming for the canvas, docs or the main sb would also be nice. So that you can do something like this or just update the theme in the canvas | docs | main sb:

pi

Note: For anyone who wants to get this to work now, here's an example repo that uses the storybook-dark-mode package.

@jonniebigodes
Copy link
Contributor

@sam-m-m wow...i can't express enough thanks for you to share a reproduction. I'll take a look at it during the weekend in conjunction to a good place for the recipes, if you don't mind. And we can go from there. If you're willing to contribute to the recipes with this one i would gladly review it and merge it.

Do you mind waiting a bit longer?

@sam-dassana
Copy link

@jonniebigodes I was able to figure it using storybook/examples/official-storybook/preview.js as a reference. I haven't updated the example repo yet. I'll try to do it this evening.

I'd love to contribute to the recipes!

@jonniebigodes
Copy link
Contributor

And we'd love for the contribution. Once it's setup and everything is working i'll let you know and we can go from there. Don't worry about including it in your repo. We still have time :)

if (typeof selected === 'string') {
api.setAddonState<string>(BACKGROUNDS_PARAM_KEY, selected);
}
api.emit(EVENTS.UPDATE, { selected, name });

Choose a reason for hiding this comment

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

@yannbf sorry to disturb you, but I can't understand why emitting of EVENTS.UPDATE was removed?

In my case I want to run some code when changing background. Previously I used the event introduced in #6561

Maybe you know is there any other way to track background change with code?

Thanks for you answer in advance!

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.

None yet

6 participants