refactor(localized-text-input): use hooks #958
Conversation
|
|
||
| static isEmpty = isEmpty; | ||
| if (hasErrorInRemainingLanguages) { |
There was a problem hiding this comment.
this replaces the getDerivedStateFromProps
There was a problem hiding this comment.
| event.target.language = this.props.language; | ||
| this.props.onChange(event); | ||
| event.target.language = props.language; | ||
| props.onChange(event); |
There was a problem hiding this comment.
Nit: could useCallback with lang and onChange as dep.
| defaultExpansionState = true; | ||
| } else if (props.defaultExpandLanguages) { | ||
| defaultExpansionState = true; | ||
| } |
There was a problem hiding this comment.
For me it would be easier to follow when inlining this at L129 and OR joining both prop values?
There was a problem hiding this comment.
Yeah it can be as simple as:
const defaultExpansionState = props.hideLanguageExpansionControls || props.defaultExpandLanguages;
|
|
||
| static getId = getId; | ||
| const toggleLanguages = React.useCallback(() => { |
There was a problem hiding this comment.
Hmm do we really need this callback? Can't we use the setter directly?
There was a problem hiding this comment.
I guess it's a micro optimization. If the expansion state doesn't change, then the function can remove memoized?
There was a problem hiding this comment.
I'm okay with removing it, just let me know 😄
There was a problem hiding this comment.
It’s not a big deal, I personally don’t see a need of wrapping a setter with another setter, as there is no extra logic involved.
|
|
||
| static isEmpty = isEmpty; | ||
| if (hasErrorInRemainingLanguages) { |
|
|
||
| static omitEmptyTranslations = omitEmptyTranslations; | ||
| if (shouldLanguagesExpand !== areLanguagesExpanded) { |
There was a problem hiding this comment.
Let's add the link to the docs here, otherwise it might be confusing to see updating the state on rendering.
montezume
force-pushed
the
ml-hook-refactor
branch
from
eed923c to
a357c60
Compare
emmenko
force-pushed
the
ml-hook-refactor
branch
from
a357c60 to
074c531
Compare
| event.target.language = props.language; | ||
| onChange(event); | ||
| }, | ||
| [props.language, onChange] |
There was a problem hiding this comment.
👆we only need to destructure onChange, for whatever reason. I suppose the lint plugin has some bugs.
Summary
Refactors the
LocalizedTextInputcontainer to use react hooks