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 Prop Types Error #14837 #14889
Fix Prop Types Error #14837 #14889
Conversation
Fixed prop types errors for Button, ButtonGroup, Modal and articlePage
|
Thank you for opening this PR! We appreciate you! For all pull requests coming from third-party forks we will need to A Forem Team member will review this contribution and get back to |
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.
Thanks for taking this on! It's looking great, but I just have a couple of small change requests - let me know if I can assist at all!
| children: PropTypes.arrayOf( | ||
| PropTypes.oneOfType([PropTypes.object, PropTypes.bool]), | ||
| ).isRequired, |
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.
refactor (blocking) - I'm not sure this is the correct fix, as we don't want booleans passed in as children of the ButtonGroup (we only want elements/Preact components 🙂 ).
Instead of changing this to silence the error, I would instead update the code using ButtonGroup and triggering the error:
forem/app/javascript/CommentSubscription/CommentSubscription.jsx
Lines 131 to 139 in e7af2bf
| {subscribed && ( | |
| <Button | |
| id="subscription-settings-btn" | |
| data-testid="subscription-settings" | |
| variant="outlined" | |
| icon={CogIcon} | |
| contentType="icon" | |
| /> | |
| )} |
We could use a ternary to avoid passing a false boolean into the component, e.g.:
{subscribed ? (
<Button
id="subscription-settings-btn"
data-testid="subscription-settings"
variant="outlined"
icon={CogIcon}
contentType="icon"
/>
) : null}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.
Exactly we should pass null @aitchiss
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 have pushed the changes please review
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.
Thanks for making those changes! ✨
What type of PR is this? (check all applicable)
Description
As per issue no. #14837 I have fixed prop types errors for Button, ButtonGroup, Modal, and articlePage components.
Related Tickets & Documents
#14837
QA Instructions, Screenshots, Recordings
I don't believe any new tests would be required for these changes.
Added/updated tests?
[optional] What gif best describes this PR or how it makes you feel?