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

Padded layout for components in storybook by default #6771

Merged
merged 6 commits into from
Jul 27, 2022

Conversation

alex-page
Copy link
Member

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2022

size-limit report 📦

Path Size
polaris-react-cjs 198.26 KB (0%)
polaris-react-esm 133.14 KB (0%)
polaris-react-esnext 188.3 KB (0%)
polaris-react-css 41.75 KB (0%)

@alex-page alex-page changed the title Fix whitespace around components in storybook Default to have whitespace around components in storybook Jul 26, 2022
@alex-page alex-page changed the title Default to have whitespace around components in storybook Default layout: padded for components in storybook Jul 26, 2022
@alex-page alex-page changed the title Default layout: padded for components in storybook Padded layout for components in storybook by default Jul 26, 2022
Comment on lines -1 to -5
<style>
.sb-show-main.sb-main-padded {
padding: 0;
}
</style>
Copy link
Member Author

@alex-page alex-page Jul 26, 2022

Choose a reason for hiding this comment

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

@kyledurand this felt like something that could be cleaned up. Let me know what you think about this PR. The other option is to make all components use layout: fullscreen by default and we could remove this and keep everything as is.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

I've not tested this to prove the layout works but I think conceptually it makes sense to keep the default of padded layout, and explicitly opt particular stories to be fullscreen. Having that extra indent makes it see things like drop shadows on the top edge of an element.

I put a note inline about how we could leverage the classes than the layout prop sets on the body to determine if any extra padding is needed instead of needing the Grid component to have an additional property.

See https://storybook.js.org/docs/react/configure/story-layout for context about the layout parameter

polaris-react/.storybook/GridOverlay/GridOverlay.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

This looks good but I do like Ben's suggestion of a purely css approach. Might be slightly tough to manage that for centered layouts. Those wouldn't likely work with the grid overlay anyway though

polaris-react/.storybook/polaris-readme-loader.js Outdated Show resolved Hide resolved
polaris-react/.storybook/GridOverlay/GridOverlay.tsx Outdated Show resolved Hide resolved
@alex-page alex-page merged commit b5ff69e into main Jul 27, 2022
@alex-page alex-page deleted the fix-component-spacing branch July 27, 2022 15:56
@github-actions github-actions bot mentioned this pull request Jul 27, 2022
@BPScott
Copy link
Member

BPScott commented Jul 27, 2022

Might be slightly tough to manage that for centered layouts. Those wouldn't likely work with the grid overlay anyway though

Yeah I'm fine with punting on centered layouts, and having the grid on them act the same as the fullscreen style grid. We don't have anything that uses the centered layout anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants