-
Notifications
You must be signed in to change notification settings - Fork 371
feat(Brand): consumed Penta updates #10045
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-10045.surge.sh A11y report: https://patternfly-react-pr-10045-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.
Approved. Great to have the light/dark switcher working on this too!
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.
Couple of quick comments below. @edonehoo do you think it'd be worth adding a description to the basic example to call out that theme support has to be manually done by the consumer, rather than it being something built into the component?
packages/react-core/src/components/Brand/examples/BrandBasic.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Brand/examples/BrandResponsive.tsx
Outdated
Show resolved
Hide resolved
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 💪
@thatblindgeye I think that makes sense here! wdyt about something simple like this?:
|
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.
Other than the above comments LGTM!
I added this, slight wording changes and placement... I added the blurb above the basic example under Examples because it applies to both examples. |
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.
LGTM!
What: Closes #9921
Mostly icon changes, added some extra code to make it responsive in different screen sizes and to make it work with light/dark mode.
Additional issues:
None