-
Notifications
You must be signed in to change notification settings - Fork 357
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
feat(Icon): add new icon sizes for penta #10030
feat(Icon): add new icon sizes for penta #10030
Conversation
Preview: https://patternfly-react-pr-10030.surge.sh A11y report: https://patternfly-react-pr-10030-a11y.surge.sh |
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.
Looks good, had a nit below about a test but not a blocker
@@ -41,16 +42,34 @@ test('sets additional custom class successfully', () => { | |||
expect(iconContainer).toHaveClass('test'); | |||
}); | |||
|
|||
Object.values(['sm', 'md', 'lg', 'xl']).forEach((size) => { | |||
Object.values([ |
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.
Nit: could we instead combine the 3 instances of this forEach into a single one, then just run all the tests that need to in here
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.
Looks good to me!
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!
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, 1 question though
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 🥇
What: Closes #9988
Additional issues:
I added the
case-anything
package to handle changing case (i.e. cameCase to kebab-case) for us, needed it for converting prop values to class names in the updated Icon tests.Convenience link: https://patternfly-react-pr-10030.surge.sh/components/icon/