BWAP-806 updating paginator component to hand textField properties #1242
Conversation
annabellewright
requested review from
tsantef,
trippygif,
yasskin and
celinrayen
| @@ -242,6 +246,7 @@ class Paginator extends React.Component<IPaginatorProps, IPaginatorState> { | |||
| 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
| onBlur={this.handleTextFieldChange} | ||
| 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.
| @@ -11,7 +11,10 @@ import { | |||
| SingleSelectDumb as SingleSelect, | |||
| ISingleSelectState, | |||
| } 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
| @@ -35,6 +38,10 @@ type IPaginatorSingleSelectProps = Partial<ISingleSelectProps>; | |||
|
|
|||
| 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
| @@ -27,6 +27,10 @@ type IPaginatorSingleSelectProps = Partial<ISingleSelectProps>; | |||
|
|
|||
| 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?
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