-
Notifications
You must be signed in to change notification settings - Fork 375
fix(createIcon): Spread className to svg to fix broken images #8954
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
Conversation
|
Preview: https://patternfly-react-pr-8954.surge.sh A11y report: https://patternfly-react-pr-8954-a11y.surge.sh |
| <svg | ||
| aria-hidden="true" | ||
| class="pf-svg" | ||
| class="pf-svg undefined" |
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.
Is the undefined class supposed to be there?
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 don't t see it on the actual component in docs. I added a check to only spread the className when not undefined I am not sure why it is happening with snapshots.
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.
Ok, I updated our version of jest. I was seeing different results in CI vs in our workspace. Updating jest fixed it.
jenny-s51
left a comment
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 @tlabaj - empty states are looking good, svg sizes as expected
| render() { | ||
| const { title, ...props } = this.props; | ||
| const { title, className, ...props } = this.props; | ||
| const classes = className ? `pf-svg ${className}` : "pf-svg"; |
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.
👌
mcarrano
left a comment
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.
@tlabaj Broken images seem to be fixed, but there is still an issue with Card View not displaying a full grid of cards.
mcoker
left a comment
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.
🚀
mcarrano
left a comment
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.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |

What: Closes #8936
Additional issues: Closes #8947
I updated the jest version. There was an issue with snaphots adding undefined attributes that was failing some tests.