feat(LoginForm): Added the PF4 LoginForm Component #874

tlabaj · 2018-11-02T01:32:32Z

What:
Added the PF4 LoginForm Component
#825

Additional issues:

patternfly-build · 2018-11-02T01:46:53Z

PatternFly-React preview: https://874-pr-patternfly-react-patternfly.surge.sh

coveralls · 2018-11-02T01:47:10Z

Pull Request Test Coverage Report for Build 3181

12 of 13 (92.31%) changed or added relevant lines in 1 file are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.2%) to 81.298%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-4/react-core/src/components/LoginPage/LoginForm.js	12	13	92.31%

Files with Coverage Reduction	New Missed Lines	%
packages/patternfly-3/patternfly-react-extensions/src/components/CatalogTileView/CatalogTileViewCategory.js	4	84.06%

Totals
Change from base Build 3154:	0.2%
Covered Lines:	3780
Relevant Lines:	4382

💛 - Coveralls

christiemolloy

This looks great Titani! CSS/UX Review:

The login button in the core example has utilities and modifiers on it to make its width responsive on different sizes if you inspect you will see this added to the end of the button classes: "pf-u-mr-lg-on-md pf-u-px-xl pf-u-align-self-stretch pf-u-align-self-flex-start-on-md" . I believe this should be added to the react example to align with what is in core.
When the fields are empty I don't see the requirement pop up of: "Please fill out this field". Are you thinking of adding that in?
Super small - can you uncapitalize the "m" in me.

Great job!!

tlabaj · 2018-11-05T18:12:35Z

@christiemolloy Thanks for the review!
I will update the example for point 1 and 3.
Can you please let me know more about item 2 and where I can find the design for this.

christiemolloy · 2018-11-05T18:18:30Z

@andybraren which design did you follow for the form in the login box with the requirement pop over?Re point 2 above.

andybraren · 2018-11-05T18:24:32Z

@christiemolloy You mean the design doc from the original Login issue? patternfly/patternfly#43

christiemolloy · 2018-11-05T18:31:35Z

@andybraren yes, could you point me to the design for the required warning pop-over "Please fill out this field", that occurs when the user clicks the log-in button and the fields are empty.

andybraren · 2018-11-05T19:14:06Z

@christiemolloy Ah, I see. That popup is Chrome's default behavior for any input with a required attribute that is empty when the form's submit button is clicked.

Changing type="button" of this demo's button to type="submit" restores the browser's behavior. I'll create a new issue to add 'type="submit" to the button in Core's Login demo as well.

Good eye Christie!

christiemolloy · 2018-11-05T21:09:10Z

Great thanks for solving that @andybraren !

christiemolloy · 2018-11-06T21:05:35Z

@tlabaj thanks for making those changes this looks great. Tiny thing i didnt catch before - could you change "Log In" to "Log in" per our new capitalization rules. Thanks! CSS/UX approved!

ibolton336

Looks good to me!

dlabaj

I don't see the Simple Login demo in the link you provide with this PR.

amarie401

Perhaps rename this to LoginForm, the demo/example isn't showing. This component will probably also need to be exported in the index.js an index.d.ts files of the components directory.

jschuler · 2018-11-08T19:30:39Z

packages/patternfly-4/react-core/src/components/LoginPage/LoginForm.js

+import flexStyles from '@patternfly/patternfly-next/utilities/Flex/flex.css';
+import displayStyles from '@patternfly/patternfly-next/utilities/Display/display.css';
+import alignmentStyles from '@patternfly/patternfly-next/utilities/Alignment/alignment.css';
+import spacingStyles from '@patternfly/patternfly-next/utilities/Spacing/spacing.css';


I guess there is a similar concern here that this will increase the size of the component too much. Could do something similar as here:
https://github.com/patternfly/patternfly-react/pull/863/files#diff-4e9f1155160426190b5a2ee57a9c0a5fR32
Or use styled-component when the PR is merged :)

jschuler · 2018-11-08T20:19:33Z

packages/patternfly-4/react-core/src/components/LoginPage/LoginForm.js

+  /** Function that handles the onChange event for the Remember Me Checkbox */
+  onChangeRememberMe: PropTypes.func,
+  /** Aria Label for the Remember me checkbox */
+  rememberMeAriaLabel: PropTypes.string.isRequired


should only be required if the rememberMeLabel is also passed in

The Checkbox component requires the Aria Label

Yes, but in this form having a checkbox with no label would not make sense, so I think if you require the aria-label you should also require the rememberMeLabel. But i could see consumers maybe not wanting to have this checkbox, so perhaps add the logic here, if rememberMeAriaLabel is passed in, then require the aria-label as well. And render the Checkbox conditionally if rememberMeLabel is used.

jschuler · 2018-11-12T17:23:58Z

packages/patternfly-4/react-core/src/components/LoginPage/examples/SimpleLoginPage.js

+        usernameValue={this.state.usernameValue}
+        onChangeUsername={this.handleUsernameChange}
+        usernameHelperTextInvalid="Unknown Username"
+        isValidUsername


Can you tie this to a state variable, and maybe in the handleUsernameChange check if username is less than 3 chars mark it as invalid? And something similar for password. So we can play with invalid state in the example. And the helperText would say something like "Username must be at least 3 characters"

Can you open an enhancement issue to add this so we can get the form merged. the invalid state is really a function of the form.

patternfly#825

packages/patternfly-4/react-core/src/components/LoginPage/LoginForm.js

tlabaj added ux review labels Nov 2, 2018

tlabaj requested review from amarie401, christiemolloy and jschuler November 2, 2018 01:32

christiemolloy reviewed Nov 5, 2018

View reviewed changes

andybraren mentioned this pull request Nov 5, 2018

Add type="submit" to the Login Demo's button patternfly/patternfly#895

Closed

tlabaj force-pushed the login_form branch from a6b84e2 to 1a1d2bb Compare November 5, 2018 21:14

amarie401 requested a review from ibolton336 November 6, 2018 20:42

christiemolloy added css approved and removed css review labels Nov 6, 2018

ibolton336 previously approved these changes Nov 7, 2018

View reviewed changes

tlabaj dismissed ibolton336’s stale review via 18a28ac November 7, 2018 16:07

tlabaj force-pushed the login_form branch from 1a1d2bb to 18a28ac Compare November 7, 2018 16:07

dlabaj reviewed Nov 7, 2018

View reviewed changes

amarie401 reviewed Nov 8, 2018

View reviewed changes

jschuler reviewed Nov 8, 2018

View reviewed changes

tlabaj force-pushed the login_form branch from 18a28ac to 2f7d41a Compare November 9, 2018 21:58

dlabaj previously approved these changes Nov 9, 2018

View reviewed changes

jschuler reviewed Nov 12, 2018

View reviewed changes

tlabaj added 6 commits November 12, 2018 16:17

feat(LoginForm): Addeds the PF4 LoginForm Component

1c71e83

patternfly#825

feat(LoginForm): Addeds the PF4 LoginForm Component

6f04703

patternfly#825

updates based on comments

f7c5d42

update snapshot test

6e1fb3e

Update example

1f246cb

Update based on review

d956f4a

tlabaj dismissed dlabaj’s stale review via d956f4a November 12, 2018 21:18

tlabaj force-pushed the login_form branch from 2f7d41a to d956f4a Compare November 12, 2018 21:18

make checkbox optional

cda9060

dlabaj previously approved these changes Nov 13, 2018

View reviewed changes

jschuler reviewed Nov 13, 2018

View reviewed changes

updates from commnets

7ae3ef1

tlabaj dismissed dlabaj’s stale review via 7ae3ef1 November 13, 2018 15:44

jschuler approved these changes Nov 13, 2018

View reviewed changes

dlabaj approved these changes Nov 13, 2018

View reviewed changes

dlabaj merged commit befeb77 into patternfly:master Nov 13, 2018

LHinson mentioned this pull request Nov 13, 2018

PF4:Login Form Component #825

Closed

feat(LoginForm): Added the PF4 LoginForm Component #874

feat(LoginForm): Added the PF4 LoginForm Component #874

Uh oh!

Conversation

tlabaj commented Nov 2, 2018

Uh oh!

patternfly-build commented Nov 2, 2018

Uh oh!

coveralls commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3181

💛 - Coveralls

Uh oh!

christiemolloy left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Nov 5, 2018

Uh oh!

christiemolloy commented Nov 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andybraren commented Nov 5, 2018

Uh oh!

christiemolloy commented Nov 5, 2018

Uh oh!

andybraren commented Nov 5, 2018

Uh oh!

christiemolloy commented Nov 5, 2018

Uh oh!

christiemolloy commented Nov 6, 2018

Uh oh!

ibolton336 left a comment

Choose a reason for hiding this comment

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

amarie401 left a comment

Choose a reason for hiding this comment

Uh oh!

jschuler Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

jschuler Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

tlabaj Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

jschuler Nov 12, 2018

Choose a reason for hiding this comment

Uh oh!

jschuler Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

coveralls commented Nov 2, 2018 •

edited

Loading

christiemolloy commented Nov 5, 2018 •

edited

Loading

jschuler Nov 12, 2018 •

edited

Loading