Skip to content

Conversation

@quarckster
Copy link
Contributor

Part of #2425

What: Added OUIA props to TextInput component

@quarckster
Copy link
Contributor Author

@redallen @karelhala test failed:

TypeScript diagnostics (customize using `[jest-config].globals.ts-jest.diagnostics` option):
    packages/patternfly-4/react-core/src/components/TextInput/TextInput.tsx:114:50 - error TS2345: Argument of type 'typeof TextInput' is not assignable to parameter of type 'ComponentClass<TextInputProps & InjectedOuiaProps, any> | FunctionComponent<TextInputProps & InjectedOuiaProps>'.
      Type 'typeof TextInput' is not assignable to type 'ComponentClass<TextInputProps & InjectedOuiaProps, any>'.
        Types of property 'defaultProps' are incompatible.
          Type '{ 'aria-label': string; className: string; isRequired: boolean; isValid: boolean; isDisabled: boolean; isReadOnly: boolean; type: string; onChange: () => any; }' is not assignable to type 'Partial<TextInputProps & InjectedOuiaProps>'.
            Types of property 'type' are incompatible.
              Type 'string' is not assignable to type '"number" | "date" | "time" | "text" | "datetime-local" | "email" | "month" | "password" | "search" | "tel" | "url"'.

    114 const TextInputWithOuiaContext = withOuiaContext(TextInput)

@quarckster quarckster changed the title [WIP] feat(TextInput): Addede OUIA for TextInput [WIP] feat(TextInput): Added OUIA for TextInput Oct 22, 2019
@tlabaj tlabaj requested review from dlabaj and redallen October 22, 2019 20:21
@vmuzikar
Copy link

vmuzikar commented Nov 4, 2019

@quarckster Hi, I just wanted to ask what's the status on this please. Any ETA? :)

@quarckster
Copy link
Contributor Author

@vmuzikar I wait for help from @redallen or @karelhala. My typescript knowledge is limited.

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Let me shed some light on this. The defaultProps are being typechecked. The type prop has a value of 'text', which typescript sees as a string rather than the union 'text' | 'otherString' | ....

The solution is to give defaultProps a type which I've done by adding } as TextInputProps; so it know that type isn't a string, but rather the same union we declared earlier.

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Update the snapshots and this is good!

@patternfly-build
Copy link
Collaborator

patternfly-build commented Nov 6, 2019

@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #3174 into master will decrease coverage by <.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3174      +/-   ##
==========================================
- Coverage   67.18%   67.18%   -0.01%     
==========================================
  Files         907      907              
  Lines       25589    25594       +5     
  Branches     2269     2271       +2     
==========================================
+ Hits        17192    17195       +3     
- Misses       7357     7358       +1     
- Partials     1040     1041       +1
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.29% <ø> (ø) ⬆️
#patternfly4 64.37% <77.77%> (-0.01%) ⬇️
Impacted Files Coverage Δ
.../react-core/src/components/TextInput/TextInput.tsx 92% <77.77%> (-3.56%) ⬇️

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 1fbb5fe...d9999cd. Read the comment docs.

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 still has some conflicts and is failing Jest test

@quarckster quarckster changed the title [WIP] feat(TextInput): Added OUIA for TextInput feat(TextInput): Added OUIA for TextInput Jan 30, 2020
@quarckster
Copy link
Contributor Author

@redallen @tlabaj tests passed

readOnly={isReadOnly}
ref={innerRef}
{...(ouiaContext.isOuia && {
'data-ouia-component-type': 'PF4/TextInput',
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the OUIA enabled components do not include the PF4/ prefix. Why are you adding it for only TextInput?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@redallen Actually I created a PR with changes for ouia enabled components #3622

Copy link
Contributor

Choose a reason for hiding this comment

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

Very well. You'll have to handle not having the PF4/ prefix for older versions in your QA suite.

redallen
redallen previously approved these changes Jan 30, 2020
@redallen
Copy link
Contributor

redallen commented Feb 3, 2020

Can't merge since there are conflicts.

@quarckster
Copy link
Contributor Author

@redallen fixed

@redallen
Copy link
Contributor

Should be rebased on our v4 branch now that #3927 is in. Closing.

@redallen redallen closed this Mar 19, 2020
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.

8 participants