Skip to content
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(helper-text): updated static and dynamic to use same icons #4246

Merged
merged 3 commits into from Jul 29, 2021

Conversation

bitwiseor
Copy link
Contributor

fixes #4205

updated static demo icons to match dynamic demo icons (specifically default and indeterminate)

@patternfly-build
Copy link

patternfly-build commented Jul 28, 2021

Preview: https://patternfly-pr-4246.surge.sh

A11y report: https://patternfly-pr-4246-coverage.surge.sh

CSS Size Report
NameCurrentPreviousDiff %
There are no changes in CSS file sizes

@mcoker
Copy link
Contributor

mcoker commented Jul 28, 2021

@mcarrano can you confirm the actual change needed for this?

Looking at the react component, the icons are the same - is that what we should be matching?

Screen Shot 2021-07-28 at 3 50 19 PM

Screen Shot 2021-07-28 at 3 50 13 PM

@mcarrano
Copy link
Member

@mcoker No. The React component has inconsistent icons. The original problem was that we can different sets of icons in each example. The goal was to use the same icons for error, success, etc. Looks like the design is saying to use the filled circle versions and not the open versions for everything. Does that help?

@bitwiseor
Copy link
Contributor Author

bitwiseor commented Jul 28, 2021

Should they be matching the icons in the React static with custom icons demo, but with filled circle versions for default and indeterminate as well?

@mcoker
Copy link
Contributor

mcoker commented Jul 28, 2021

@bitwiseor sorry for the confusion. I met with @mcarrano and we determined these should be the icons used for both static and dynamic

Screen Shot 2021-07-28 at 4 24 55 PM

We'll also want to make sure everywhere the component is used is updated. That should be:

  • Helper text examples page
  • Helper text demos page
  • Password Strength demo

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!! 🥳

@mcoker mcoker changed the title fix(helper-text): updated static demo icons to match dynamic demo icons fix(helper-text): updated static and dynamic to use same icons Jul 29, 2021
@mcoker mcoker merged commit 45ca3c9 into patternfly:main Jul 29, 2021
@mcoker
Copy link
Contributor

mcoker commented Jul 29, 2021

@doruskova can you confirm the icons here are what you were expecting to see? https://patternfly-pr-4246.surge.sh/components/helper-text

@patternfly-build
Copy link

🎉 This PR is included in version 4.125.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@doruskova
Copy link

@mcoker Yes! They're correct. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update icons in Helper text component
5 participants