BWAP-806 updating paginator component to hand textField properties#1242
BWAP-806 updating paginator component to hand textField properties#1242annabellewright wants to merge 7 commits intomasterfrom
Conversation
annabellewright
requested review from
celinrayen,
trippygif,
tsantef and
yasskin
| event: React.FocusEvent | React.FormEvent; | ||
| } | ||
| ): void => { | ||
| console.log(pageNum); |
There was a problem hiding this comment.
might want to remove the log statement here unless it's intentional
| onSubmit={this.handleTextFieldChange} | ||
| isDisabled={isDisabled} | ||
| isDisabled={isDisabled || textFieldProps.isDisabled} | ||
| {...textFieldProps} |
There was a problem hiding this comment.
this change of order would result in textFieldProps.isDisabled taking precedence over isDisabled.
if not intentional change, would leave the spread operator on the line above
There was a problem hiding this comment.
Good callout. Updated and added a test case as an example for this.
| } from '../SingleSelect/SingleSelect'; | ||
| import TextField, { ITextFieldProps } from '../TextField/TextField'; | ||
| import TextField, { | ||
| ITextFieldProps, |
There was a problem hiding this comment.
nitpick: are both ITextFieldProps/ITextFieldPropsWithPassThroughs still used? else can be removed from the imports
There was a problem hiding this comment.
Removed with ITextFieldPropsWithPassThroughs
| type ShowTotalObjects = (count: number) => string; | ||
|
|
||
| interface IExtendedTextFieldProps extends Omit<ITextFieldProps, 'value'> { | ||
| value?: string | number; |
There was a problem hiding this comment.
is this being done just to allow number as a type on value?
There was a problem hiding this comment.
Updated to only allow string as an optional value
| }} | ||
| TextField={{ | ||
| value: 3, | ||
| isDisabled: true, |
There was a problem hiding this comment.
unless covered by another storybook example - also include an example if isDisabled: false (assume it is covered as the default value)
There was a problem hiding this comment.
Covered by new storybook example
| onBlur={this.handleTextFieldChange} | ||
| onSubmit={this.handleTextFieldChange} | ||
| isDisabled={isDisabled} | ||
| isDisabled={isDisabled || textFieldProps.isDisabled} |
There was a problem hiding this comment.
can this be covered in a test case?
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
merge master
BWAP-806 update to master
tsantef
left a comment
tsantef
left a comment
There was a problem hiding this comment.
What specific problem in the UI is this change trying to solve, what's the use case? I'm just curious because this component has been around for a long time without this modification.
|
|
||
| type ShowTotalObjects = (count: number) => string; | ||
|
|
||
| interface IExtendedTextFieldProps extends Omit<ITextFieldProps, 'value'> { |
There was a problem hiding this comment.
Why do you need to redefine the value field?
6 participants
PR Checklist
Storybook can be viewed here:
Manually tested across supported browsers
[ x] Unit tests written (
commonat minimum)[ x] PR has one of the
semver-labelsTwo core team engineer approvals
One core team UX approval