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

feat(Form): add form group label info & docs(Form): add password strength demo #6053

Merged
merged 9 commits into from Aug 2, 2021

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Jul 19, 2021

What: Closes #6002 & #5819

Adds labelInfo property to FormGroup.
Adds new unit test, example and integration test.
Also updates some duplicate ids in the examples.

Update: Adds password strength demo

@patternfly-build
Copy link
Contributor

patternfly-build commented Jul 19, 2021

@kmcfaul kmcfaul changed the title feat(Form): add form group label info feat(Form): add form group label info & docs(Form): add password strength demo Jul 26, 2021
@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jul 26, 2021

Updated to include the password strength demo.

@mattnolting @mcoker I'm not sure why, but nesting a HelperText element inside pf-c-form__group-label pf-m-info overrides the color granted by the HelperText variant (error/warning/success), instead always showing black. Any thoughts? Edit: Actually figured this out, was missing the container.

@mcarrano Is there a set way to calculate password strength that we want for this demo? It was unclear in the design what the rules are for the strength of a password. Currently I have it counting how many times the 3 minimum required rules are being met and with more uppercase/special characters or digits the stronger the password becomes.

@mcarrano
Copy link
Member

@mcarrano Is there a set way to calculate password strength that we want for this demo? It was unclear in the design what the rules are for the strength of a password. Currently I have it counting how many times the 3 minimum required rules are being met and with more uppercase/special characters or digits the stronger the password becomes.

@kmcfaul I think what you are doing is probably fine to demo this. We don't have any particular standard for predicting password strength that we are recommending through PatternFly. @ajacobs21e is there any particular rules you would recommend applying for the purpose of this demo?

@ajacobs21e
Copy link
Member

@kmcfaul @mcarrano Red Hat uses an algorithm to output a score. I can find out what it is if you'd like.
https://github.com/dropbox/zxcvbn is an open source one recommended by infosec

nicolethoen
nicolethoen previously approved these changes Jul 26, 2021
@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jul 27, 2021

Is it worth bringing in another library for just a demo? The code shows how the UI will change and users can implement their own validation/strength algorithm in place of the demo function.

@mcarrano
Copy link
Member

@kmcfaul I agree that it does not make sense to pull in an external library. Maybe there should just be mention that this demo is only to show how to reflect status in the UI, but does not intend to specify any particular validation technique.

@ajacobs21e do you think it would make sense to reflect the InfoSec recommendation in the design doc that relates to this or do you think PatternFly should just steer clear of that?

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jul 27, 2021

@mcarrano Updated the example docs to include a short note. Let me know if that looks good (once it finishes building).

@mcarrano
Copy link
Member

@kmcfaul how about just adding one more sentence: "We expect consumers will substitute their own, more robust, validation algorithm."

@ajacobs21e
Copy link
Member

Yeah that would be great to mention their recommendation. thanks!

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jul 29, 2021

@mcarrano @ajacobs21e Updated the example disclaimer

mcarrano
mcarrano previously approved these changes Jul 29, 2021
Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for adding the disclaimer @kmcfaul

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! Just one comment about the validation text.

packages/react-core/src/demos/PasswordStrength.md Outdated Show resolved Hide resolved
mcoker
mcoker previously approved these changes Jul 29, 2021
tlabaj
tlabaj previously approved these changes Jul 30, 2021
nicolethoen
nicolethoen previously approved these changes Jul 30, 2021
dlabrecq
dlabrecq previously approved these changes Jul 30, 2021
mcarrano
mcarrano previously approved these changes Jul 30, 2021
Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Thanks @kmcfaul !

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

I think this makes sense for mouse and keyboard users, but I think that we should follow the same pattern that we have for our validated form example, particularly for screen reader users.

The input should have an aria-describedby attribute that points to the helper text, and it should be aria-invalid when validation isn't met. It looks like your example has that, but the aria-describedby points to an id that doesn't exist, and it also looks like the aria-invalid never updates. Then the helper text should be inside of an aria-live="polite" to let the screen reader know that the text will change/update.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Jul 30, 2021

@jessiehuff Updated with your feedback. Let me know if the aria-live region is updating properly when it finishes building (I wrapped the HelperText with the div).

tlabaj
tlabaj previously approved these changes Aug 2, 2021
Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

I tested in VO, JAWS, and NVDA, and I'm seeing kind of unreliable results. I was only able to get the helper text to announce once in VO. When I compare the demo to our form invalid example, I think the things that might be affecting it are that aria-invalid never updates (it's always "false"), and the id that the aria-describedby points to is on a different element than the one with the aria-live. Perhaps that's causing the issue? I don't want to hold up the release though for this so we could create an issue to refine this more. At least the helper text seems discoverable in the normal DOM order on its own.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Aug 2, 2021

Pushed one last update moving the aria-live and fixing the aria-invalid not being updated. Hopefully that puts the PR in a decent state to merge, we can open a followup to better fit the screenreaders if needed

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

LGTM! Great job! 😄

@tlabaj tlabaj merged commit 0f11c0a into patternfly:main Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Form - add info text to group label
9 participants