-
Notifications
You must be signed in to change notification settings - Fork 353
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(Text area): Add resize disable feature #11383
Conversation
Preview: https://patternfly-react-pr-11383.surge.sh A11y report: https://patternfly-react-pr-11383-a11y.surge.sh |
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.
This looks good, just non-blocking comment below. Could we also add a test that checks that no additional class is added for the new orientation value? Looks like we're also missing a test that checks for the pf-m-resize-both
modifier class when orientation is both, but that can be a followup if need be.
@@ -8,7 +8,8 @@ import { FormControlIcon } from '../FormControl/FormControlIcon'; | |||
export enum TextAreResizeOrientation { | |||
horizontal = 'horizontal', | |||
vertical = 'vertical', | |||
both = 'both' | |||
both = 'both', | |||
disabled = 'disabled' |
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.
Can't recall if there was a conversation regarding this, but was using "none" considered instead of "disabled", to match the CSS value that would normally be used? Not a blocker since "disabled" is still accurate, just wondering if people may think a modifier class will be applied like in other instances odf a disabled modifier.
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 actually debated between the two names. I started with none
. I can change it back.
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.
👍 changes look good and tested fine, nice job.
i'm on the fence on whether we should add a specific example for it, since we do call out horizontal and vertical resize orientations explicitly. that makes it seem like we should have examples for all available options, especially none
which i personally think will be a very popular use case (one could make a strong case that unresizable should really be the default). up to you though, not enough for me to not approve.
@tlabaj thanks for adding the example, looks good... merging |
What: Closes #11130