-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[ButtonGroup] Add orientation prop #18762
[ButtonGroup] Add orientation prop #18762
Conversation
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.
Good start, could you:
- Fix the build
- Update the TypeScript definitions
- Add a demo about this prop
Thanks
@material-ui/core: parsed: +0.25% , gzip: +0.12% Details of bundle changes.Comparing: a8cd858...267b7f3
|
I catch errors at ButtonGroup.md I already add the prop orientation but the test continue failing, what other things could I be missing? |
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.
Running yarn prettier
should help with the static fail.
@SandraMarcelaHerreraArriaga Try: |
@SandraMarcelaHerreraArriaga I believe the last missing part is a new demo, we are almost there :). |
Thank you!! I started to check how others contributors make a demo :) |
fix orientation value in buttonGroupjs Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
Hello, I have been added in the current demo but I'm not sure why now two tests continue failing... |
Thanks for the demos, what do you thinking of breaking it up into smaller demos? Outside visual regression tests, I don't think that we need to render all the size or orientation variants |
Ohh I see, got it. let me try :) and fix all the size or orientation variants that I made before |
@@ -301,7 +301,7 @@ function Demo(props) { | |||
!demoOptions.hideHeader && | |||
demoOptions.defaultCodeOpen !== false && | |||
jsx !== demoData.raw && | |||
jsx.split(/\n/).length <= 15; | |||
jsx.split(/\n/).length <= 16; |
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.
@mbrookes one step at the time 👼.
@SandraMarcelaHerreraArriaga Thanks |
May I also suggest to add some responsiveness by automatically triggering the orientation prop to |
@aress31 Thanks for the suggestion. I don't think that we should bake this feature in. I think that an external wrapper, possibly in userland would be better to start with. |
Closes #17884