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 #8128: addon-centered clashing with addon-docs for react #8388

Merged

Conversation

mrijke
Copy link
Contributor

@mrijke mrijke commented Oct 11, 2019

The centered addon was clashing when used in combination with the
docs addon. When on the docs page, the centered style should not be applied.
Note that this commit only fixes it for React.

Issue:
See #8128, when on the DocsPage of the docs addon, the component is being centered in the page.

What I did

Adapted the suggestion from #8128 (comment) to check if the current view is the Docs view.

Note that this only fixes the React variant of this addon. I don't have much experience with the other frameworks, but if requested I wouldn't mind trying out fixing those as well.
Also had to disable the eslint check for no-undef for the window usage, tried to make it a bit better with checking if window is not undefined, but open for better suggestions :)

The centered addon was clashing when used in combination with the
docs addon. When on the docs page, the centered style should not be applied.
Note that this commit only fixes it for React.
@vercel
Copy link

vercel bot commented Oct 11, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/f77tpmmh5
✅ Preview: https://monorepo-git-fork-mrijke-fix-8128-addon-centered-w-79f137.storybook.now.sh

@shilman
Copy link
Member

shilman commented Oct 11, 2019

Hmmm @CodeByAlex were you looking at this at one point? This seems like a pretty hacky solution.

@CodeByAlex
Copy link
Member

CodeByAlex commented Oct 12, 2019

I was looking into ways to optimize addon centered but not this specific issue. I think I brought it up though I’m terms of angular but it was fixed. Whatever solution we come up with, the same should be applied to this issue for zooming #8389

@mrijke
Copy link
Contributor Author

mrijke commented Oct 14, 2019

@shilman Yeah I agree, this is not the most beautiful solution, but I was hitting this issue and saw people proposed something, just wanted to start a PR to get the ball rolling.
I'm all open for other suggestions, if I have some more time I'll try and see if I can somehow better determine whether the docs view is active.
Or if you have another idea of how to approach this problem, please share some pointers :)

@stale stale bot added the inactive label Nov 4, 2019
@@ -5,6 +5,17 @@ import parameters from './parameters';
import styles from './styles';

function centered(storyFn: () => ReactNode) {
/* eslint-disable no-undef */
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a useViewmode hook would be a good idea?

Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen I think that would be very useful, independent of this PR

@stale stale bot removed the inactive label Nov 7, 2019
@storybookjs storybookjs deleted a comment from stale bot Nov 16, 2019
@ndelangen ndelangen added this to the 5.4.0 milestone Nov 16, 2019
@stale
Copy link

stale bot commented Dec 7, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Dec 7, 2019
@shilman shilman modified the milestones: 5.4.0, 6.0.0 Dec 16, 2019
@stale stale bot removed the inactive label Dec 16, 2019
@stale
Copy link

stale bot commented Jan 10, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jan 10, 2020
@loonywizard
Copy link

Hi! Looks like this PR is ready, do you have any plans for releasing it? In our project we are waiting for this change

This is not urgent for us, but we're looking forward to use addon-centered in our Storybook

Thank you!

@stale stale bot removed the inactive label Jan 10, 2020
@ndelangen
Copy link
Member

@shilman considering the centered decorator will be likely deprecated in 6.0.0 for this:
#9229

We might as well fix this in 5.3 maybe?

@shilman
Copy link
Member

shilman commented Jan 11, 2020

@ndelangen I'm fine with patching this into 5.3 after the release is out

@stale
Copy link

stale bot commented Feb 1, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Feb 1, 2020
@stale stale bot removed the inactive label Feb 1, 2020
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.

5 participants