-
Notifications
You must be signed in to change notification settings - Fork 364
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
refactor: [M3-7098] - Create Stack
component
#9830
refactor: [M3-7098] - Create Stack
component
#9830
Conversation
title: 'Components/Spacer', | ||
}; | ||
|
||
export default meta; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. This component is opinionated about layout composition. Increasing its usage means adding empty divs everywhere for building very basic layouts, which can be achieved with a simple flexbox space-between
container (and occasional grouping of children). Not a fan of adding empty markup everywhere for this purpose. It also could make responsive development less intuitive.
I thought you were already complaining about too many divs 😆 ! What is your reasoning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just appreciate the convenience of it but I can see that side of things too.
My main complaint is Cloud Manager's overuse of MUI Grid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main complaint is Cloud Manager's overuse of MUI Grid.
While I agree, I don't think this solves it. Besides Grid has responsive handling baked in, this component will need some customisation and extra sx with breakpoints to achieve the same. All that being said it would be nice to have a session on layout composition and spend more time discussing when and where to use what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this solves it
Agree with this
Stack
and Spacer
componentsStack
component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to test it in Storybook some more but I noticed a couple of typos
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
I did not observe any visual defects with these changes. Running Storybook locally worked as expected. I think this component is a great addition to our component library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! nice addition.
Put an item to discuss layout composition with the team and will include this one as well
Description 📝
Stack
to our "component library"Changes 🔄
Stack
componentPreview 📷
How to test 🧪
Storybook
yarn storybook
Stack
story for styling, functionality, and spellingCloud Manager
As an Author I have considered 🤔