-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[ToggleButtons] Add toggle buttons #10144
Conversation
This is still WIP but wanted to give y'all a chance to make some early comments. Still need to flesh out the docs and specs. I have a much better appreciation for how hard it is to build 'on spec'. So much of the spec is left undefined and open to interpretation. I tried to follow general style and spacing guidelines I've seen in the other components in the spec but this one specifically does not have redlines or text opacity guidelines. Right now the behavior is completely controlled - it is incumbent on the developer to manage the selected state of both the buttons and the group. I want to add 'auto' support on the group to show selected if any of the child buttons are selected as well as manage an aggregate Love any feedback anyone would like to offer before I make this ready for production. |
return ( | ||
<div className={ classes.toggleContainer }> | ||
<ToggleButtonGroup selected={ alignment.length > 0 }> | ||
<ToggleButtonWithValue value="left" values={ alignment } onClick={ this.handleToggleAlignment }><FormatAlignLeft /></ToggleButtonWithValue> |
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.
jsx attributes shouldn't be spaced: prop={value}
I don't think it'll pass npm run lint
like so (try to have your editor format/eslint --fix on save)
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.
Thanks for the note. This is currently WIP. I'll be sure to lint everything when it's totally ready. (normally have editor do it automatically but haven't set it up for this project).
ac1096c
to
94a1688
Compare
|
||
return React.cloneElement(child, { | ||
selected, | ||
onChange: exclusive ? this.handleExclusiveChange : this.handleChange, |
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.
In this implementation the signature for the call to onChange
changes slightly based on the exclusive
prop. When exclusive onChange
is called with a single value and when false it is called with an array of values. I don't have a really strong opinion on best approach. The adaptive implementation is really convenient for consumers of the component but the variable signature might have surprising results later. Happy to entertain other perspectives.
63a3aca
to
27d2f3f
Compare
@phallguy Hey, thank you for this pull-request! I'm sorry I haven't provided a feedback earlier. I'm focusing on addressing the last issues before releasing v1. Reviewing this pull and merging can sensibly delay v1. I won't make it a priority until we ship v1. |
Sounds good. Looking forward to the 1.0 ship! |
@phallguy The lab package is up and running ( |
@phallguy How goes it? 👋 Just checking in to see if you're still working on this, or need any help moving it to the lab package? |
Thanks for your patience. Been distracted with some other work. I haven't spent any time looking into moving into the lab. Is there any resource or guides available for submitting to the lab? |
@phallguy No worries, just checking in, no rush at alI. I need to write that up, but basically it's exactly the same as for the main package, except the files go in the You could look at the Slider PR to get an idea of the structure: |
Ok seems easy enough. I'll try and grab some time this week. |
a3b8fb9
to
b8c9f1d
Compare
This comment has been minimized.
This comment has been minimized.
b8c9f1d
to
636dbfb
Compare
84d3f38
to
2fa762f
Compare
2fa762f
to
b02145d
Compare
@phallguy My sincere apologies that it has taken so long to merge this. I had allowed time for @oliviertassinari to review the PR, but he has been tied up with support, bug fixes, and performance enhancements for core components, both before and since releasing v1, so hasn't had the opportunity, so I'm going to merge it, and take the responsibility for anything I've overlooked. Thanks for your hard work pulling this together! |
No worries I really appreciate all the work you guys have done. Ping me if you need help cleaning anything up. Glad to help. |
@phallguy We keep forgetting to ask people, but would you be interested in any MUI logo stickers as a small token of appreciation? |
Yeah that'd be pretty cool. |
Cool. Ping me a note on gitter with your address and how many you'd like. |
Adding support for toggle buttons.