-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(structured-list): remove unused prop #5592
fix(structured-list): remove unused prop #5592
Conversation
/** | ||
* Specify whether a border should be added to your StructuredListWrapper | ||
*/ | ||
border: PropTypes.bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to wrap this in a deprecate
block as lame as that is otherwise if someone is passing in border
it might get spread onto <section>
with other
accidentally 😞 This could cause React to complain about the extra attribute showing up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep the rest of the border
code as well? Or is keeping the prop definition wrapped in a deprecate enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tw15egan I doubt we need to keep the class name code. As long as we have:
const { children, selection, className, ariaLabel, border: _border, ...other } = this.props;
I think it'd be fine so it doesn't get caught up in other
. The rest of the code can totally be dropped I think (alongside the deprecate bit for the prop type)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I've updated it, let me know if the changes look good 👍
Deploy preview for carbon-components-react ready! Built with commit 0974603 https://deploy-preview-5592--carbon-components-react.netlify.com |
Deploy preview for carbon-elements ready! Built with commit d76eacf |
Deploy preview for carbon-elements ready! Built with commit 0974603 |
202a685
to
8817e43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - Thanks @tw15egan!
* fix(structured-list): remove unused prop * fix(structured-list): wrap prop in deprecate block * fix(structured-list): remove border from other Co-authored-by: Josh Black <[email protected]>
* fix(structured-list): remove unused prop * fix(structured-list): wrap prop in deprecate block * fix(structured-list): remove border from other Co-authored-by: Josh Black <[email protected]>
* fix(structured-list): remove unused prop * fix(structured-list): wrap prop in deprecate block * fix(structured-list): remove border from other Co-authored-by: Josh Black <[email protected]>
Closes #5590
Removes all references to the
border
prop. There were no associated styles with it, and it had no effect on the component. We can put it behind a deprecation flag, but I'm not sure that is necessary since this just seems like an old prop that was part of the original implementation that was never removed when we made the large changes tov10
Changelog
Removed
border
in theStructuredList
componentTesting / Reviewing
Ensure the component renders properly