Storybook: Addon to wrap stories in max-width div #45134
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
aaronrobertshaw
commented
Oct 20, 2022
|
Size Change: +134 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Oct 20, 2022
storybook/decorators/with-wrapper.js
Outdated
|
|
||
| const Wrapper = styled.div` | ||
| max-width: 248px; | ||
| padding: 16px; |
There was a problem hiding this comment.
Should we also show an outline ? It would help to read the padding better
There was a problem hiding this comment.
I did ponder adding a border myself but erred towards minimising changes from how the current component stories looked. I'll add one tomorrow 👍
There was a problem hiding this comment.
For sure there are pros and cons — I though an outline / border could help also with the look of the sidebar?
There was a problem hiding this comment.
A couple small things:
- I believe the correct metrics are 280-(16*2) = 248px? (248px is the internal content width, after padding)
- I'm hoping we can keep this add-on to have a singular purpose for now — so, just max-width and nothing else. (No padding, no borders, etc). Would that be ok?
- One of the reasons for point 2 ☝️ is that almost every Storybook decorator we have is a "wrapper". So it would be good to be clear about what each wrapper is for. Could we change all the naming to be more specific, like
WithMaxWidthWrapper,context.globals.maxWidthWrapper, etc?
We could also consider injecting an indicator div (like the margin checker) so nobody gets confused whether or not a max-width is being applied. No strong opinion on this at the moment though. We could add something later if we do find it confusing.
There was a problem hiding this comment.
Thanks, all good suggestions.
- Padding has been removed
- Max width has been corrected
- No border added
- Add-on has been renamed to indicate a singular purpose
- Indicator has also been added
- Styles copied from the margin checker
- I think it's good to make it easy to know what max-width is being applied
mirka
reviewed
Oct 20, 2022
storybook/decorators/with-wrapper.js
Outdated
|
|
||
| const Wrapper = styled.div` | ||
| max-width: 248px; | ||
| padding: 16px; |
There was a problem hiding this comment.
A couple small things:
- I believe the correct metrics are 280-(16*2) = 248px? (248px is the internal content width, after padding)
- I'm hoping we can keep this add-on to have a singular purpose for now — so, just max-width and nothing else. (No padding, no borders, etc). Would that be ok?
- One of the reasons for point 2 ☝️ is that almost every Storybook decorator we have is a "wrapper". So it would be good to be clear about what each wrapper is for. Could we change all the naming to be more specific, like
WithMaxWidthWrapper,context.globals.maxWidthWrapper, etc?
We could also consider injecting an indicator div (like the margin checker) so nobody gets confused whether or not a max-width is being applied. No strong opinion on this at the moment though. We could add something later if we do find it confusing.
|
Thanks for the reviews! I think everything has been cleaned up now if you'd like to give it another once-over 🙂 |
mirka
approved these changes
Oct 21, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related:
What?
Adds a quick means to wrap a story in a div reflecting the max-width and padding constraints of the WordPress sidebar.
Why?
This addon will allow us to remove the ad hoc wrappers on some components, e.g.
BorderBoxControl, that set amax-widthmatching the WordPress editor sidebars. This way, the component can stretch the full width of the viewport by default illustrating how the component adapts to fill space.When developing components, this addon will make it quick and easy to constrain components to a real-world scenario, such as the width of our editor sidebars.
How?
Testing Instructions
UnitControlstory and confirm the control stretches the full width of your canvasWP Sidebaroption for the new "Wrapper" addon.Screenshots or screencast
Screen.Recording.2022-10-20.at.10.38.13.am.mp4