refactor ButtonWIthIcon to replace ButtonSearch #81
Conversation
clairesunstudio
requested review from
avrilpearl,
smurrayatwork,
pmallett-mc and
mrjmd
clairesunstudio
changed the title
| if (icon.type.name === 'SvgSearch') { | ||
| classNames += ' ma__button-search'; | ||
| } | ||
| console.log(type.name) |
There was a problem hiding this comment.
This needs to be removed.
| let classNames = this.props.classes.join(' '); | ||
| if (this.props.canExpand) { | ||
| const { classes, canExpand, expanded, capitalized, iconSize, iconColor, icon, type } = this.props | ||
| let classNames = classes.join(' '); |
There was a problem hiding this comment.
Instead of joining this at this line, why not join after all the logic for classNames is done? That way you can just push new items onto the classes array instead of appending potentially empty strings.
There was a problem hiding this comment.
sure will update
| @@ -21,28 +21,33 @@ class ButtonWithIcon extends React.Component { | |||
| } | |||
| } | |||
| render() { | |||
| let classNames = this.props.classes.join(' '); | |||
| if (this.props.canExpand) { | |||
| const { classes, canExpand, expanded, capitalized, iconSize, iconColor, icon, type } = this.props | |||
There was a problem hiding this comment.
Missing a semicolon here.
| const buttonProps = { | ||
| type: 'submit', | ||
| type: type, |
There was a problem hiding this comment.
This should just be the type variable (object property shorthand).
| /** The aria-label property is used to provide the label to any assistive | ||
| * technologies. This is useful if the text value is not descriptive of the | ||
| * button's functionality. */ | ||
| ariaLabel: PropTypes.string, |
There was a problem hiding this comment.
Remove trailing comma.
| @@ -62,7 +62,7 @@ HeaderSearch.propTypes = { | |||
| /** The placeholder text for the input */ | |||
| placeholder: PropTypes.string, | |||
| /** The Search button */ | |||
| buttonSearch: PropTypes.shape(ButtonSearch.propTypes), | |||
| buttonSearch: PropTypes.shape(ButtonWithIcon.propTypes), | |||
There was a problem hiding this comment.
This prop is no longer being used in render. I don't believe that's intentional?
There was a problem hiding this comment.
buttonSearch is accessed on line 49
There was a problem hiding this comment.
I forgot to add the props before...
|
@petermallett @smurrayatwork the reason i reverted the className handling change because for some reason the component rendered more than 1 time and each time the array gets more pushes and results in the following: |
pmallett-mc
dismissed
their stale review
something weird happened to this PR just now...
clairesunstudio
force-pushed
the
react/buttonSearch-refactoring
branch
from
3382338 to
58be836
Compare
|
@petermallett i did something to it by mistake... it's reverted |
| .add( | ||
| 'ButtonSearch', | ||
| withInfo(`<div>${buttonWithIconReadme}</div>`)(() => { | ||
| const props = { |
There was a problem hiding this comment.
do you want all these available as knobs for buttonsearch - i would assume search icon etc should not be an option to play with in knobs cause this is a specific variant that needs that icon type
This PR refactors ButtonWithIcon to replace and archive the ButtonSearch component. It's a step towards creating a fully configurable button pattern.
It also fixes the spacing between the text and the icon in the search button in the Search UI. See the visual differences in Search:
before:
after: