[BD-46] feat: replace custom submit and reset btns with IconButton - #2719
[BD-46] feat: replace custom submit and reset btns with IconButton#2719khudym wants to merge 2 commits into
Conversation
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thanks for the pull request, @khudym! When this pull request is ready, tag your edX technical lead. |
3eb5319 to
a6f30af
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2719 +/- ##
==========================================
- Coverage 92.83% 92.83% -0.01%
==========================================
Files 235 235
Lines 4241 4240 -1
Branches 1029 1029
==========================================
- Hits 3937 3936 -1
Misses 300 300
Partials 4 4 ☔ View full report in Codecov by Sentry. |
a6f30af to
62f39b3
Compare
62f39b3 to
a72d149
Compare
|
@khudym We should also try to simplify <SearchField
...
icons={{
clear: Close,
submit: Search,
}}
/>It's also important not to make a breaking change out of this, one option to do this is to use withDeprecatedProps HOC. I've played around with it a bit, and I think something like this will allow consumers to use new props without breaking old behaviour (this is just a POC, the actual implementation might be cleaner) SearchField.propTypes = {
...
icons: PropTypes.shape({
submit: PropTypes.elementType.isRequired,
clear: PropTypes.elementType,
})
};
SearchField.defaultProps = {
...,
icons: {
clear: Close,
submit: Search,
},
};
export default withDeprecatedProps(SearchField, 'SearchField', {
icons: {
deprType: DeprTypes.FORMAT,
expect: value => typeof value === 'object' && !React.isValidElement(value.submit) && !React.isValidElement(value.clear),
transform: value => {
const newValue = { ...value };
if (React.isValidElement(value.submit)) {
newValue.submit = value.submit.props.src;
}
if (React.isValidElement(value.clear)) {
newValue.clear = value.clear.props.src;
}
return newValue;
},
message: 'It should be an object with "clear" and "submit" keys that contains Icon instances as values.',
},
});After this you can pass icon components in both ways // old variant, consumers will get a warning about deprecation of this usage
<SearchField
icons={{
clear: <Icon src={Close} />,
submit: <Icon src={Search} />,
}}
/>
// new variant, intended behaviour
<SearchField
icons={{
clear: Minus,
submit: Add,
}}
/>Can you try to do this as well? (think we'll also need to implement this for |
|
@adamstankiewicz Are we concerned about breaking change here or not. I like @viktorrusakov 's solution. What stops me from doing that is potential breaking change since we change component's API. |
|
@adamstankiewicz, |
|
@monteri we'll want to make sure the commit uses the breaking change format https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-both--and-breaking-change-footer |
|
We plan to release all breaking changes at the same time, a new PR is open #2842 |
|
@khudym Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Replace custom Searchfield buttons with IconButton
Issue
Deploy Preview
Deploy
Merge Checklist
exampleapp?wittjeffandadamstankiewiczas reviewers on this PR.Post-merge Checklist