Skip to content
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(textarea): support isDisabled #5107

Merged
merged 1 commit into from
Dec 7, 2020
Merged

feat(textarea): support isDisabled #5107

merged 1 commit into from
Dec 7, 2020

Conversation

boaz0
Copy link
Member

@boaz0 boaz0 commented Nov 8, 2020

What: Closes #5068

It is a breaking change. If someone is using disabled in the props then after updating with this change Typescript will show an error message saying disabled is not a valid props of TextArea.

@patternfly-build
Copy link
Contributor

patternfly-build commented Nov 8, 2020

redallen
redallen previously approved these changes Nov 9, 2020
@@ -10,11 +10,13 @@ export enum TextAreResizeOrientation {
both = 'both'
}

export interface TextAreaProps extends Omit<HTMLProps<HTMLTextAreaElement>, 'onChange' | 'ref'> {
export interface TextAreaProps extends Omit<HTMLProps<HTMLTextAreaElement>, 'disabled' | 'onChange' | 'ref'> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of this and I think it warrants finally having a "breaking type change" discussion I've been postponing.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add the isReadOnly prop. That is what the consumer seems to be looking for. Please see my comment in the issue. We want the consumers to still be able to use the native disabled or readOnly` attributes so we don't break them. Please add integration test to verify.

kmcfaul
kmcfaul previously approved these changes Nov 11, 2020
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also update the demo app please.

@boaz0
Copy link
Member Author

boaz0 commented Nov 11, 2020

@tlabaj done! 👍

@codecov-io
Copy link

Codecov Report

Merging #5107 (6622bcb) into master (ddab4a4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5107   +/-   ##
=======================================
  Coverage   52.40%   52.41%           
=======================================
  Files         541      541           
  Lines        9923     9925    +2     
  Branches     3696     3698    +2     
=======================================
+ Hits         5200     5202    +2     
  Misses       4059     4059           
  Partials      664      664           
Flag Coverage Δ
patternfly4 52.41% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/react-core/src/components/TextArea/TextArea.tsx 95.00% <100.00%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f59f7c...6622bcb. Read the comment docs.

kmcfaul
kmcfaul previously approved these changes Nov 13, 2020
@@ -124,6 +124,12 @@ export class TextAreaDemo extends React.Component<{}, TextAreaState> {
validated={validated}
aria-label="text area example 5"
/>
<Text>Disabled text area </Text>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you update the cypress test to verify expected behaviour.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one just needs cypress tests updated.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0
Copy link
Member Author

boaz0 commented Dec 3, 2020

Hi @tlabaj, I added tests to cypress as you requested. Sorry for the late response 🙇

@redallen redallen merged commit 9ebcfe9 into patternfly:master Dec 7, 2020
@boaz0
Copy link
Member Author

boaz0 commented Dec 7, 2020

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextArea isDisabled property is called readOnly?
6 participants