Button component #711
Button component #711
Conversation
dzonidoo
requested review from
MarkLark86,
petrjasek and
thecalcc
and removed request for
petrjasek,
MarkLark86 and
thecalcc
dzonidoo
changed the title
* add navigation * changes after review * fix lint * chages after review
|
hi @dzonidoo can you update the PR so we can merge it? |
* add navigation * changes after review * fix lint * chages after review
assets/components/Buttons.tsx
Outdated
| } | ||
|
|
||
| interface IPropsReset extends IPropsButtonBase { | ||
| type?: 'reset', |
There was a problem hiding this comment.
I guess here the type must be reset so should be probably just type: 'reset',
I'm also wondering if we have a usecase where we wouldn't use it with onClick, might be better to make it mandatory if not to get this checked.
same applies to submit
|
|
||
| interface IPropsSubmit extends IPropsButtonBase { | ||
| type: 'submit', | ||
| onClick?(event: React.MouseEvent<HTMLButtonElement, MouseEvent>): void; |
There was a problem hiding this comment.
I would still make this mandatory, if we need to put there empty function in case this is not needed it's probably better than forgetting it when it's needed. same goes for reset button
| import {IconButton, IPropsSize, IPropsVariant} from './IconButton'; | ||
|
|
||
| interface IProps { | ||
| testId?: string, |
There was a problem hiding this comment.
I see this is data-test-id on other components, would be good to make it consistent
There was a problem hiding this comment.
so change the name?
| action={action} | ||
| isVisited={action.visited && action.visited(this.props.user, this.props.item)} |
There was a problem hiding this comment.
why are we dropping this functionality?
NHUB-464
Checklist
lodash.getwith optional chaining for modified code segments