feat(LoginPage): add LoginPage component with storybook and tests.

[Ron-Lavi]

affects: patternfly-react

Add LoginPage component with storybook and tests. fix #80

Pattern
Storybook

Basic Login Page:

Social Login Page

[jeff-phillips-18]

Theoretically, if we mimic the HTML from http://www.patternfly.org/pattern-library/application-framework/login-page/#code#login-layout-markup we shouldn't need additional less/sass.

[Ron-Lavi]

@jeff-phillips-18 I added some styles to make the page less "jumpy" when the page change dynamically.

[lizsurette]

@LaViro This is looking super great. I'm excited to use this in my product :)

I just did a quick review from a UX point of view (FYI @serenamarie125 @jgiardino) and noticed a few small things that I think would be great to update as you continue working on this.

1. When there aren't any errors, the padding of the form elements seems to be a bit too much. (Between email address and password fields, and remember me checkbox. And then again between the log in button and need an account link)

2. In the PF base implementation example, the language that is selected (English in this case) is actually highlighted as selected when the user drops down the menu. Is this something we can do here?

3. A few little typo/text nitpicks: Updates should be made that we use a lower case “a” in "Email address" and uppercase "I" in "Log In".

4. I like the way you've broken this up into 3 sections in storybook. Any chance the section names could be updated to be a bit clearer for each use case? Maybe “Basic Login Page”, “Login Page with Errors”, and “Login Page with Social Options”? Totally open to other suggestions, just thought this would be clearer.

5. On the social options page, the less link needs an updated icon pointing up rather than down.

6. On the social options page, the responsive state is missing the separator bar between the log in button and social options.

Great work on this! I hope this feedback helps.

Liz

[Ron-Lavi]

@lizsurette Thanks for the feedback :)
I will try to answer to some of your thoughts.

1. Input errors: I agree that the padding are big, but as I have added validation to the form and errors have started to appear, the interaction seems too annoying..
   You can try to press the CapsLock while in the password field or submit the form while empty, and than type something else in the inputs, and submit again.. I can make another version where the errors will collapse and you can see the difference.

2. Language Picker dropdown highlight - no problem, we can add it !

3. Letter-Case updates - will be fixed.

4. Rename storybooks: Thanks, I guess this names need to be clearer :)

5. Arrow Up on "Less" button - wil be fixed

6. Seperator bar - will be fixed

I guess you will see my updates soon, I would be happy to know what you think about the input's errors.

Thanks, Ron

[lizsurette]

@LaViro Your updates all look great.

I totally get what you are saying about the shifting of the form when you have to add in error messages. I think this is something that should be decided at the PF Core level before just making a change in PF-react, but it might be nice to hear from others who are closer to the process.

@mcarrano @LHinson @jgiardino - If there are potential improvements to Core identified while doing work in a framework repo, what is the typical process for discussing these ideas? Maybe a topic for the coordinating across repos discussion? This specific one is around the best way to allow space for potential form errors and I don't think would just apply to the Login page, but that's the current context.

Thanks,
Liz

[mcarrano]

@lizsurette and @LaViro Thanks for raising this issue. I do see what you mean, and it's difficult to evaluate this type of behavior with the core examples where the validation code is not included. @lizsurette would you mind entering an issue against patternfly-design for this? That will ensure this gets on the agenda for our weekly design meeting. I think it raises a general question about field spacing on forms and whether it is better to keep the spacing compact and let the spacing change to insert the error message or whether spacing should always account for a single row of text below the field. Sounds like you guys are recommending the later?

[Ron-Lavi]

@mcarrano The validation code DO exist in the managed social and basic LoginPage.

Cases when error / warning will be shown:

• Submit while input is empty.
• Submit while email is invalid.
• Submit while password is too short.
• On password while Caps Lock is on.
• Form is valid but there's a server error.

As for the input fields, I believe that fading an error on a fixed height space will be better.
For form errors which raised by the server, I find collapse better.

Thanks :)

[Ron-Lavi]

@gregsheremeta @lizsurette I have added the additionalFields prop
for inserting more fields under the password field.

[jgiardino]

Hi @LaViro, thanks again for contributing this! You've done a great job implementing one of our more complex components! And you've added a lot of nice touches. 👌

Login page background style

When I look at the basic login page using dev tools, I see that there's a background image defined as an inline style:
<div class="login-pf" style="background-image: url(&quot;static/media/login-screen-bg.bb6b114e.jpg&quot;);">

But if I disable that style, it looks like there's also a background image defined for .login-pf that displays. Is there a reason why the background image is added as an inline style? Can we remove that, or were there other issues related to this?

Login card input height ( for solving the jumping errors/warnings over an input )

My understanding from past discussions about this is that the implementation as it is now works as expected. I would love to see forms handle error messages in a more graceful way, but I think implementing that type of feature in a one-size-fits-all way that accommodates translation and the possibility of multiple error cases per field is not an easy thing to solve.

Login form error ( color and margin )

What you have looks correct.

Social Login divider ( margin and border )
Social Login columns ( margin on small screens )

When you click the "More" link to show more options, there's a larger margin between the options that were visible and the options that become visible.

Instead of having two separate ul.login-pf-social.list-unstyled.login-pf-social-double-col elements, can there just be one, with the extra <li> elements added to the original one? In the core html/css example there's an additional class login-pf-social-all on the <ul> that will hide the extra options. But I don't think it's a big deal if that solution doesn't work with this implementation, because I think we need to modify the margin anyway...

The core html/css implementation doesn't include the divider, but the design does. I think for now, we can assume that the design is correct, and keep the divider like you have it, but there aren't styles in the core css to produce the same layout that's shown in the design. And right now, the spacing in this implementation doesn't fully match the design:

My suggestion would be to add the following to the pf-react css, which will also fix the margin issue that I mention above:
.login-pf-page .login-pf-social {margin-top: 0;}

This will not match the design document exactly, but I think it's close enough, and we need to create a separate issue anyway to track down why there's a difference and figure out what the expected solution should be. So we can revisit this layout later if needed when that issue is resolved.

Also in this implementation, I see that you animate the transition when clicking the Less/More link, which is a really nice touch. But it seems to stick when expanding. It shows the first 3, pauses, then expands all the way to show the 4th one. Do you see that in your build? Is there anyway to adjust that animation?

page alert ( margin )

I can't find an example of this in core. I'd say it's fine as it is for now, and we can revisit this element in core and update later as needed.

One other thing I noticed -
The warning that displays for the caps lock key doesn't always display when expected. When I have focus in the password field, and toggle the caps lock key, keypress event seems to be captured later than it should.

[Ron-Lavi]

Wow @jgiardino, thanks for the detailed review !

Is there a reason why the background image is added as an inline style?

It gives a way for the developers to insert a background from props without dealing with css:

it looks like there's also a background image defined for .login-pf that displays.

I think pf should remove this image from the .login-pf since it won't fit every product.

instead of having two separate ul.login-pf-social.list-unstyled.login-pf-social-double-col elements, can there just be one, with the extra <li> elements added to the original one?

I see three options:

• leave it just as it is right now with two classes.
• remove the collapsing div, and try to create collapse animation by css rules - not easy.
• remove the collapse.

and maybe add a class to PF that do collapse ?

providing gifs of the options:
Regular

the regular also solves the css problem with the margin between sections:

Collapsed

The warning that displays for the caps lock key doesn't always display when expected.

I was writing about this issue here above, it works fine for me,
I am using Fedora and chrome v66.

I saw that there are problems with CapsLock detection on Mac and some browsers,
going to check for a solution that will match most users.

[jgiardino]

Thanks for the clarification about the background image. That makes a lot of sense! 😉 I opened an issue in patternfly core to capture this question, plus a couple of others that came up during review: patternfly/patternfly#1125

I was just chatting with @mcarrano about the issues with the responsive state (i.e. the issue with the extra margin and the issue with the animation not being a smooth transition from start to finish when clicking More). @mcarrano's recommendation for resolving this is to remove the More/Less toggle from the responsive state, since there isn't much benefit to the user in collapsing the extra options on a mobile device.

Is it possible to remove the toggle from the responsive state and keep it for the other? @mcarrano's feedback was that if it's problematic to do this, then his recommendation is to remove the toggle altogether.

I saw that there are problems with CapsLock detection on Mac and some browsers,
going to check for a solution that will match most users.

If you are having difficulty reproducing this issue and finding a solution for it, we can capture this in a follow-up issue for someone with a mac to look into.

[matthewcarleton]

hey @LaViro This is looking great! Thanks for putting in all the effort here. I'm going to do the final CSS review so we can get this moving while Jenn is away on PTO next week.
In addition to what she pointed out above I noticed one other thing.
The language selector is not aligned to the right like we have in the core example. From looking at the code in the react implementation it looks like a slight difference in how it was built. It just a matter of adding <span class="filter-option pull-left"> around the "english text and adding .btn-group to the wrapping div with .bootstrap-select. If this isn't clear just let me know!

[Ron-Lavi]

@matthewcarleton @jgiardino
thank you for your comments ! I fix the styling problems mentioned above,
can we continue with this PR and adjust the styles after the issues you've created will be closed?

@jeff-phillips-18 I think I've fixed the capslock behavior, can you check if it works on mac ? thanks :)

[matthewcarleton]

Hey @LaViro, great work, thanks!
I'd suggest pulling out the

---

and that should resolve the margin issue. The line for mobile should not be in the design and the design will be updated to reflect this :)

What are your thoughts on removing the more/less toggle? Is this difficult to do? I like @mcarrano's suggestion of removing it altogether too if that more reasonable from a technical stand point.

I think if we address those two things we can move forward with this.

[priley86]

This is looking really nice to me @LaViro ! I will add a couple of comments inline, but overall very happy w/ this.

One thing you will definitely want to do before merging is export your Login components in the main patternfly-react barrel file (patternfly-react/src/index.js). It would be nice to see all of your individual components/patterns exported via a component index file (components/LoginPage/index.js). See Table index.js for example.

[priley86]

we do not need to duplicate these logos here in patternfly-react. They can be consumed/re-exported directly from Patternfly since we are directly dependent on PF.

That being said, I think we can change this to :

import dropbox from 'patternfly/dist/img/dropbox-logo.svg';
import facebook from 'patternfly/dist/img/facebook-logo.svg';
import fedora from 'patternfly/dist/img/fedora-logo.png';
import git from 'patternfly/dist/img/git-logo.svg';
import github from 'patternfly/dist/img/github-logo.svg';
import google from 'patternfly/dist/img/google-logo.svg';
import instagram from 'patternfly/dist/img/instagram-logo.png';
import linkedin from 'patternfly/dist/img/linkedin-logo.svg';
import openID from 'patternfly/dist/img/open-id-logo.svg';
import skype from 'patternfly/dist/img/skype-logo.svg';
import stackExchange from 'patternfly/dist/img/stack-exchange-logo.svg';
import twitter from 'patternfly/dist/img/twitter-logo.svg';

and delete all of the new logo assets coming from PF...

[priley86]

We started tracking all keycodes for accessibility in src/helpers.js , KEY_CODES constant. I think it is best to have one constant for these.

[priley86]

(same as above)

[priley86]

Definitely a higher order component (pattern). The LoginPageWithTranslation and LoginCardWithValidation components seem to do a lot of internal state management. I think I would generally handle that in the application, but since you have done such a nice job breaking down the components, this should not be an issue (and will probably help a lot of consumers needing less fine grain detail). I'm OK proceeding here if others are ;-)

[Ron-Lavi]

Thank you so much @priley !
The consumer of this component can decided whether they
want the component with validation or not,
by setting the form: { validate } prop.

[Ron-Lavi]

thanks @matthewcarleton,
I have removed the horizontal line in the social-login-page,
about the 'more/less toggle', I have already fix that :)
so now it doesn't appear when screen is xs, and there is only one ul container for the list.

[matthewcarleton]

awesome! @jeff-phillips-18 this is good to go from a CSS perspective.

[dgutride]

@LaViro, @LHinson, from talking with @priley86, we have two necessary changes:
I've 👍 them above (removing logos and fixing the barrel file to include the necessary exports). After that, we can track the keycode changes using a separate issue.

Please add a comment and tag @priley86 and I once those two changes have been made and this is ready to go in.

[priley86]

Filed #530 for any follow-up enhancements. This looks good to go from my standpoint!