feat(login): adds a password strength demo #4145

srambach · 2021-06-11T21:11:36Z

Adds an example login page showing interactive password strength using helper text

patternfly-build · 2021-06-11T21:15:38Z

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

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

CSS Size Report

Name	Current	Previous	Diff %
There are no changes in CSS file sizes

mcoker

This looks great!! A couple of questions for @mcarrano

This is built as a demo - there were no enhancements to the login component to support this PR. The way the helper text that is adjacent to the label when it's above the text input is using a split layout in the demo. We may get variation in use in products if the way we put helper text adjacent to the label isn't built into the form or login component itself. This would also need to be built differently if a horizontal form were used. This seems OK, but I wanted to confirm whether the intent was for it to be purely a demo or if we want to build support for helper text by the form label into the form or login component.
We typically do not use beta components in other component examples/demos, so we don't give the impression the component is ready to use. It's definitely a login page demo, but I wonder if it belongs under the helper text demos since it's beta?

@srambach the ultimate goal for the helper text component is to replace the current helper text elements we have in individual components. In this demo, the helper text component is placed after the text input with a margin from a utility class. I think we could go ahead and use/add an element in the form component that has the proper spacing for helper text in relation to a form field, and place the helper text component inside of that. Looking at the .pf-c-form__helper-text element, that seems generic enough (maybe except for the font-size declaration) to be able to support putting the helper text component inside. The font-size won't conflict here though since the helper text component's font-size is the same. WDYT?

src/patternfly/components/Login/examples/Login.md

srambach · 2021-06-14T16:08:49Z

@mcoker there seems to be a problem here because .pf-c-form__helper-text creates a <p> tag, but you cannot nest the pf-c-helper-text with its <div>s inside a p tag.

mcarrano · 2021-06-14T16:37:09Z

This is built as a demo - there were no enhancements to the login component to support this PR. The way the helper text that is adjacent to the label when it's above the text input is using a split layout in the demo. We may get variation in use in products if the way we put helper text adjacent to the label isn't built into the form or login component itself. This would also need to be built differently if a horizontal form were used. This seems OK, but I wanted to confirm whether the intent was for it to be purely a demo or if we want to build support for helper text by the form label into the form or login component.

I think this is OK. I had thought at times whether we should define a unique Password component that includes all of these capabilities, but seems like that's overkill. Do you agree @mcoker ? I think we can provide guidelines around usage of the password strength in the the design guidelines to drive consistent usage.

We typically do not use beta components in other component examples/demos, so we don't give the impression the component is ready to use. It's definitely a login page demo, but I wonder if it belongs under the helper text demos since it's beta?

I see your point, @mcoker , but I don't think anyone would look for this there. Is it possible to mark this specific example/demo as Beta since it uses a beta component?

ajacobs21e · 2021-06-14T17:14:06Z

This isn't part of the login page. It would be for an account creation form, not a login form once the password is already created.
The strength on the top right (weak, etc) shouldn't be shown if one of the password requirements below has failed. Only after all are checked would we then show the password strength, because it doesn't matter if it's the strongest password in the world if it doesn't meet a requirement.

mcarrano · 2021-06-14T17:56:01Z

That's a great point @ajacobs21e . I had wrongly been thinking of this as associated with the password field on Login. In that case, I think we should have two separate demos. One that is associated with the Helper Text component and shows how to use that for field validation and separately to demonstrate how to add password strength. I'm not sure where we add the later. @mcoker is this just a demo of how to add this to a form field? Do you think we would create something unique called Password strength and add to the Demo section?

mcoker · 2021-06-14T21:26:07Z

@mcarrano yep, we're just demoing how to use the helper text with a form field. I think the password strength demo could be a unique demo, especially since it's tied to multiple components. And since it's in it's own section, we could create a few examples showing the various states.

mcoker · 2021-06-14T21:53:49Z

there seems to be a problem here because .pf-c-form__helper-text creates a
tag, but you cannot nest the pf-c-helper-text with its
s inside a p tag.

@srambach nice catch! Looking at https://www.patternfly.org/v4/components/form, all of the helper text is a <div> and it's coming from <FormGroup> - https://github.com/patternfly/patternfly-react/blob/master/packages/react-core/src/components/Form/FormGroup.tsx.

Though there is a <FormHelperText> component that generates a <p> - https://github.com/patternfly/patternfly-react/blob/master/packages/react-core/src/components/Form/FormHelperText.tsx

If the preferred use of helper text in a form is to use the helperText* props on <FormGroup>, I'm in favor of changing <FormHelperText> to a <div> to allow for a more flexible element to put helper text in, in case users are using it to define their helper text. It seems like it would be good to be consistent in the element we use in <FormGroup> and <FormHelperText>.

For the purposes of this PR, I think it's fair to assume .pf-c-form__helper-text can be either a <p> or <div> and we could use a <div> here.

I'm also curious from @kmcfaul, @tlabaj, and @jessiehuff on that change, as well as how they would envision using the helper text component to add helper text to a form field.

tlabaj · 2021-06-15T16:30:58Z

@mcoker Right now the react component accepts any React node for the helperText prop. The consumer can pass a string if they wish. In that case the helper text would not be wrapped in <p> tag. (see example here).
If they they pass a <FormHelperText> node, they would get the <p> tag, (see example here)

mcoker

Just a few small comments, though a bigger issue is the use of the split to have the helper text beside the form label. Left a comment about that. If we can't figure out a solution, I'd be fine with just omitting that part of the demo (the text beside the label for the strength text) and following up with that next sprint.

mcoker · 2021-06-16T16:07:24Z

src/patternfly/components/Form/form-helper-text.hbs

  {{#if form-helper-text--attribute}}
    {{{form-helper-text--attribute}}}
  {{/if}}>
  {{> @partial-block}}
-</p>
+</{{#if form-helper-text--type}}


nit - typically we'll make this a single line like the opening tag for consistency an easy whitespace management.

mcoker · 2021-06-16T16:17:43Z

src/patternfly/demos/PasswordStrength/examples/PasswordStrength.md

+### Initial state
+```hbs
+  {{#> form}}
+    {{#> form-group}}


Looks like we're missing {{#> form-group-label}} and {{#> form-group-control}} around the form controls. And the helper text will go in form-group-control.

mcoker · 2021-06-16T17:35:10Z

src/patternfly/demos/PasswordStrength/examples/PasswordStrength.md

+    {{#> form-group}}
+      {{#> split split--modifier="pf-m-gutter"}}
+        {{#> split-item split-item--modifier="pf-m-fill"}}
+          {{#> form-label form-label--attribute='for="invalid-login-demo-form-password"' required="true"}}Password{{/form-label}}


One thing we do a lot is give the {{#> form}} block a form--id="helper-text-invalid", then do form-label--attribute=(concat 'for="' form--id '-password"'), so we can use that anywhere we need a unique string that's used multiple places in an example, then use that same code in each example with a unique form--id.

mcoker · 2021-06-16T17:35:55Z

src/patternfly/demos/PasswordStrength/examples/PasswordStrength.md

+        {{/split-item}}
+      {{/split}}
+      {{#> input-group}}
+        {{#> form-control controlType="input" input="true"  form-control--attribute='type="search" id="invalid-login-demo-form-password" name="invalid-login-demo-form-password" aria-label="Password input" value="" placeholder="Password"'}}


type="search" should be type="text"?

mcoker · 2021-06-16T17:38:32Z

src/patternfly/demos/PasswordStrength/examples/PasswordStrength.md

+  {{/form}}
+```
+
+### Valid, but weak password


nit - just to match the next example

Suggested change

### Valid, but weak password

### Valid, weak password

mcoker · 2021-06-16T17:43:32Z

src/patternfly/demos/PasswordStrength/examples/PasswordStrength.md

+```hbs
+  {{#> form}}
+    {{#> form-group}}
+      {{#> split split--modifier="pf-m-gutter"}}


Looking at this split a little more, I don't know that we can do this in react. The <FormGroup> component takes a label for the label, and its children are passed into the form group control element. I'm wondering if we need to add a new form element for info to be displayed beside the label like this, and potentially support for what that would look like in a horizontal form layout. wdyt?

mcarrano

This looks god to me now from a design perspective. Thanks @srambach !

jessiehuff

This looks pretty good! My only concern is that I believe this helper text will end up being dynamic so screen reader users will need to be updated with this information the same way that sighted users are. I imagine that it would follow the same pattern as our invalid form example. The only difference that I wonder about is the password strength text.

srambach · 2021-06-29T14:19:04Z

This looks pretty good! My only concern is that I believe this helper text will end up being dynamic so screen reader users will need to be updated with this information the same way that sighted users are. I imagine that it would follow the same pattern as our invalid form example. The only difference that I wonder about is the password strength text.

@jessiehuff do you think that aria-live="polite" should be added to the info by default here, or do you think that should ultimately be handled by using dynamic helper text?

mcoker

LGTM! Just a couple of small things.

src/patternfly/demos/PasswordStrength/examples/PasswordStrength.md

…h.md Co-authored-by: Michael Coker <[email protected]>

mcoker

LGTM! Thanks!!

patternfly-build · 2021-07-09T21:49:59Z

🎉 This PR is included in version 4.121.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

srambach requested review from mcoker, ajacobs21e and mcarrano June 11, 2021 21:11

mcoker requested changes Jun 11, 2021

View reviewed changes

src/patternfly/components/Login/examples/Login.md Outdated Show resolved Hide resolved

srambach force-pushed the 4018-password-strength-demo branch 2 times, most recently from fae6574 to 55de8ac Compare June 16, 2021 15:47

mcoker reviewed Jun 16, 2021

View reviewed changes

mcarrano approved these changes Jun 16, 2021

View reviewed changes

jessiehuff reviewed Jun 21, 2021

View reviewed changes

srambach added 5 commits July 2, 2021 08:55

feat(password-strength): create new demo

92dce8f

feat(password-strength): address comments

2a1216b

feat(password-strength): add missing form-group-label

06128db

feat(password-strength): add missing form-group-control

0fc791a

feat(password-strength): use form group label info

6eeba94

srambach force-pushed the 4018-password-strength-demo branch from 569289b to 6eeba94 Compare July 2, 2021 13:58

srambach requested a review from mcoker July 2, 2021 14:27

mcoker requested changes Jul 2, 2021

View reviewed changes

srambach and others added 4 commits July 2, 2021 15:12

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

36bc689

…h.md Co-authored-by: Michael Coker <[email protected]>

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

da8b193

…h.md Co-authored-by: Michael Coker <[email protected]>

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

75ebf3d

…h.md Co-authored-by: Michael Coker <[email protected]>

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

c20d694

…h.md Co-authored-by: Michael Coker <[email protected]>

srambach and others added 4 commits July 2, 2021 15:14

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

82d3adf

…h.md Co-authored-by: Michael Coker <[email protected]>

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

188287e

…h.md Co-authored-by: Michael Coker <[email protected]>

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

28d5235

…h.md Co-authored-by: Michael Coker <[email protected]>

Update src/patternfly/demos/PasswordStrength/examples/PasswordStrengt…

a5d70b9

…h.md Co-authored-by: Michael Coker <[email protected]>

mcoker approved these changes Jul 2, 2021

View reviewed changes

mcoker changed the title ~~feat(login): adds a password strength example~~ feat(login): adds a password strength demo Jul 9, 2021

mcoker merged commit 93dd7c0 into patternfly:main Jul 9, 2021

patternfly-build added the released label Jul 9, 2021

srambach deleted the 4018-password-strength-demo branch June 7, 2022 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(login): adds a password strength demo #4145

feat(login): adds a password strength demo #4145

srambach commented Jun 11, 2021 •

edited

Loading

patternfly-build commented Jun 11, 2021 •

edited

Loading

mcoker left a comment •

edited

Loading

srambach commented Jun 14, 2021

mcarrano commented Jun 14, 2021

ajacobs21e commented Jun 14, 2021

mcarrano commented Jun 14, 2021

mcoker commented Jun 14, 2021

mcoker commented Jun 14, 2021 •

edited

Loading

tlabaj commented Jun 15, 2021 •

edited

Loading

mcoker left a comment

mcoker Jun 16, 2021

mcoker Jun 16, 2021

mcoker Jun 16, 2021

mcoker Jun 16, 2021

mcoker Jun 16, 2021

mcoker Jun 16, 2021

mcarrano left a comment

jessiehuff left a comment

srambach commented Jun 29, 2021

mcoker left a comment

mcoker left a comment

patternfly-build commented Jul 9, 2021

feat(login): adds a password strength demo #4145

feat(login): adds a password strength demo #4145

Conversation

srambach commented Jun 11, 2021 • edited Loading

patternfly-build commented Jun 11, 2021 • edited Loading

mcoker left a comment • edited Loading

Choose a reason for hiding this comment

srambach commented Jun 14, 2021

mcarrano commented Jun 14, 2021

ajacobs21e commented Jun 14, 2021

mcarrano commented Jun 14, 2021

mcoker commented Jun 14, 2021

mcoker commented Jun 14, 2021 • edited Loading

tlabaj commented Jun 15, 2021 • edited Loading

mcoker left a comment

Choose a reason for hiding this comment

mcoker Jun 16, 2021

Choose a reason for hiding this comment

mcoker Jun 16, 2021

Choose a reason for hiding this comment

mcoker Jun 16, 2021

Choose a reason for hiding this comment

mcoker Jun 16, 2021

Choose a reason for hiding this comment

mcoker Jun 16, 2021

Choose a reason for hiding this comment

mcoker Jun 16, 2021

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

srambach commented Jun 29, 2021

mcoker left a comment

Choose a reason for hiding this comment

mcoker left a comment

Choose a reason for hiding this comment

patternfly-build commented Jul 9, 2021

srambach commented Jun 11, 2021 •

edited

Loading

patternfly-build commented Jun 11, 2021 •

edited

Loading

mcoker left a comment •

edited

Loading

mcoker commented Jun 14, 2021 •

edited

Loading

tlabaj commented Jun 15, 2021 •

edited

Loading