feat(LoginPage): added isPasswordRequired prop #11673

Mash707 · 2025-03-13T17:31:28Z

patternfly-build · 2025-03-13T17:35:30Z

Preview: https://patternfly-react-pr-11673.surge.sh

A11y report: https://patternfly-react-pr-11673-a11y.surge.sh

thatblindgeye

This looks good to me. Would you want to try writing a couple tests for this prop? One to check that the password field is required by default, and another to check that it is not required when isPasswordRequired is false.

Mash707 · 2025-03-17T14:25:42Z

I'll work on writing the tests. Should I include the changes in this PR or open a followup? Also we don't have any examples showcasing the false state of the prop.

thatblindgeye · 2025-03-17T14:53:45Z

You can include it in this PR. @andrew-ronaldson would we want to include an example for such a simple difference?

andrew-ronaldson · 2025-03-17T18:16:38Z

Not sure if it needs an example. We can document the new prop with the existing example as a start

Mash707 · 2025-03-17T18:17:07Z

@thatblindgeye I did some research and found out that passing the prop in FormGroup only changes the visual state of the field. This is not a solution we are looking for since it removes the ASTERISK symbol visually but the password field is still required.

patternfly-react/packages/react-core/src/components/Form/FormGroup.tsx

Lines 55 to 67 in 54e11ce

    
               <LabelComponent className={css(styles.formLabel)} {...(!isGroupOrRadioGroup && { htmlFor: fieldId })}> 
        
                 <span className={css(styles.formLabelText)}>{label}</span> 
        
                 {isRequired && ( 
        
                   <span className={css(styles.formLabelRequired)} aria-hidden="true"> 
        
                     {' '} 
        
                     {ASTERISK} 
        
                   </span> 
        
                 )} 
        
               </LabelComponent> 
        
               <>&nbsp;&nbsp;</> 
        
               {isValidElement(labelHelp) && <span className={styles.formGroupLabelHelp}>{labelHelp}</span>} 
        
             </> 
        
           );

I am currently trying to find the component which handles the input state of password field.

Mash707 · 2025-03-17T18:27:10Z

isPasswordRequired = true

isPasswordRequired = false

In both cases password is still required.

So the changes I have made in this PR are only visually updating the field

Mash707 · 2025-03-17T18:45:24Z

patternfly-react/packages/react-core/src/components/LoginPage/LoginForm.tsx

Lines 86 to 96 in 54e11ce

    
           const passwordInput = ( 
        
             <TextInput 
        
               isRequired 
        
               type={passwordHidden ? 'password' : 'text'} 
        
               id="pf-login-password-id" 
        
               name="pf-login-password-id" 
        
               validated={isValidPassword ? ValidatedOptions.default : ValidatedOptions.error} 
        
               value={passwordValue} 
        
               onChange={onChangePassword} 
        
             /> 
        
           );

Needed to pass down the prop here too.

Mash707 · 2025-03-17T18:49:41Z

I have added test case to ensure required field in password is present. Can't add test for not required since LoginPageDemo.tsx has isPasswordRequired=true by default.

Mash707 · 2025-03-17T18:58:39Z

Currently I have not explicitly mentioned isPasswordRequired prop in any of the examples or demo(true by default). Should I include it?

thatblindgeye · 2025-03-17T19:33:48Z

@Mash707 thanks for catching and looking into that.

For the test, we'd actually want to add a unit test for LoginForm specifically (in the packages/react-core/src/components/LoginPage/__tests__ directory).

We could add some verbiage to the MD file for the examples before the actual code between lines 32 and 34 in the MD). @edonehoo wdyt of something like "While the username field will always be required, you can make the password field optional by passing the isPasswordRequired property. By default a password will also be required." as an example description?

edonehoo · 2025-03-17T20:13:30Z

@thatblindgeye how do you feel about swapping things around a little:

"By default, a login page requires users to enter both a username and a password into their respective fields. The username must always be a required field, but you can can make the password optional by passing the isPasswordRequired property to the <LoginForm>."

Mash707 · 2025-03-18T20:33:20Z

I have added the test cases and also the explanatory text for the isPasswordRequired field.
I have squashed the commits.

thatblindgeye · 2025-03-19T14:25:11Z

packages/react-core/src/components/LoginPage/__tests__/LoginForm.test.tsx

@@ -48,4 +48,16 @@ describe('LoginForm', () => {
    const { asFragment } = render(<LoginForm isShowPasswordEnabled />);
    expect(asFragment()).toMatchSnapshot();
  });
+
+  test('LoginForm with isPasswordRequired', () => {


For the test descriptions, typically we try to keep a format of "Renders...."

So for these 2, maybe something like "Renders LoginForm with password field required by default" and "Renders LoginForm with password field not required when isPasswordRequired set to false"

I have updated the test description.

thatblindgeye · 2025-04-08T13:07:59Z

packages/react-core/src/components/LoginPage/__tests__/LoginForm.test.tsx

+    expect(passwordField).toHaveAttribute('required');
+  });
+
+  test('Renders LoginForm with password field not required when isPasswordRequired set to false', () => {
+    render(<LoginForm isPasswordRequired={false} />);
+    const passwordField = screen.getByLabelText(/password/i);
+    expect(passwordField).not.toHaveAttribute('required');


Last change request I swear 😆 Rather than using the toHaveAttribute, we should be able to use toBeRequired() instead for these tests

Yeah that makes more sense, will make the changes.

patternfly-build · 2025-04-11T19:14:15Z

Your changes have been released in:

@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]
@patternfly/[email protected]
@patternfly/[email protected]

Thanks for your contribution! 🎉

* feat(LoginPage): added isPasswordRequired prop * updated test names * mades changes to test file

thatblindgeye reviewed Mar 17, 2025

View reviewed changes

Mash707 force-pushed the add-password-prop-to-login-page branch from 48e3c69 to 27cfe4d Compare March 17, 2025 18:44

Mash707 force-pushed the add-password-prop-to-login-page branch 2 times, most recently from b00a23b to 23d75ae Compare March 18, 2025 20:45

thatblindgeye reviewed Mar 19, 2025

View reviewed changes

Mash707 force-pushed the add-password-prop-to-login-page branch from 23d75ae to 49b904a Compare March 19, 2025 19:46

Mash707 requested a review from thatblindgeye March 19, 2025 19:57

thatblindgeye requested changes Apr 8, 2025

View reviewed changes

thatblindgeye requested a review from kmcfaul April 8, 2025 13:10

Mash707 added 3 commits April 8, 2025 19:37

feat(LoginPage): added isPasswordRequired prop

06dcf05

updated test names

ecd1c4b

mades changes to test file

99c09c9

Mash707 force-pushed the add-password-prop-to-login-page branch from 49b904a to 99c09c9 Compare April 8, 2025 14:17

Mash707 requested a review from thatblindgeye April 8, 2025 14:40

thatblindgeye approved these changes Apr 8, 2025

View reviewed changes

kmcfaul approved these changes Apr 11, 2025

View reviewed changes

thatblindgeye merged commit 0658fb3 into patternfly:main Apr 11, 2025
13 checks passed

thatblindgeye mentioned this pull request Apr 11, 2025

6.2.0 patch release patternfly/patternfly-infra-issues#137

Closed

8 tasks

dlabaj pushed a commit that referenced this pull request Apr 17, 2025

feat(LoginPage): added isPasswordRequired prop (#11673)

1e923c0

* feat(LoginPage): added isPasswordRequired prop * updated test names * mades changes to test file

Mash707 deleted the add-password-prop-to-login-page branch May 21, 2025 19:28

feat(LoginPage): added isPasswordRequired prop #11673

feat(LoginPage): added isPasswordRequired prop #11673

Uh oh!

Conversation

Mash707 commented Mar 13, 2025

Uh oh!

patternfly-build commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Mash707 commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye commented Mar 17, 2025

Uh oh!

andrew-ronaldson commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mash707 commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mash707 commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mash707 commented Mar 17, 2025

Uh oh!

Mash707 commented Mar 17, 2025

Uh oh!

Mash707 commented Mar 17, 2025

Uh oh!

thatblindgeye commented Mar 17, 2025

Uh oh!

edonehoo commented Mar 17, 2025

Uh oh!

Mash707 commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatblindgeye Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

Mash707 Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Mash707 Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patternfly-build commented Apr 11, 2025

Uh oh!

Uh oh!

patternfly-build commented Mar 13, 2025 •

edited

Loading

Mash707 commented Mar 17, 2025 •

edited

Loading

andrew-ronaldson commented Mar 17, 2025 •

edited

Loading

Mash707 commented Mar 17, 2025 •

edited

Loading

Mash707 commented Mar 17, 2025 •

edited

Loading

Mash707 commented Mar 18, 2025 •

edited

Loading