-
Notifications
You must be signed in to change notification settings - Fork 358
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
feat(Select): add disabled flag for component #2678
Conversation
Requires core to be updated to v2.26 to get styling, in #2668 |
PatternFly-React preview: https://patternfly-react-pr-2678.surge.sh |
awaiting core merge for disabled styling
…ead form sub-elements
e1180c2
to
fcfb875
Compare
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.
just a typo to fix
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.
great job @kmcfaul! 🥇
packages/patternfly-4/react-core/src/components/Select/Select.tsx
Outdated
Show resolved
Hide resolved
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.
LGTM
constructor(props) { | ||
super(props); | ||
this.options = [ | ||
{ value: 'Alabama', disabled: false }, |
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'd remove the disabled keys from the options array in the two added examples for clarity purposes
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 was thinking they same thing. But thought it was fine. You are right though, it would be more clear.
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.
LGTM! :)
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.
How about instead of adding more examples just to demonstrate this, add a checkbox to an existing example that can toggle isDisabled?
@jschuler , I think the examples are fine as is. They match core. |
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.
LGTM
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Adds isDisabled flag to select.
Refer to issue: #2366