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

Components: Adds LoggedOutFormContainer component #1284

Closed
wants to merge 1 commit into from

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Dec 4, 2015

In an effort to factor the logged out link that shows at the bottom of the form at /start/account/user and to allow reuse of the logged out form styles, this PR refactors the signup/logged-out-form component and brings much of its functionality to client/components/logged-out-form-container component.

At this point, I would like to solicit feedback from designers (cc @rickybanister) as well as devs ( cc @scruffian, @drewblaisdell, @lezama, @roccotripaldi ) to get feedback.

Questions:

  • I'm not sure I like logged-out-form-container for a component name. Do you have other suggestions?
  • What are your thoughts on this approach? Should we scrap and go another route, or is this on the right path?
  • The links are now left-aligned, which matches @rickybanister's people management mocks. This looks a bit off to me with a single link (ex. the login-form screenshot). Should single links be center aligned?

While this PR is working, it should not be merged as it is a try PR and will require other work before it is ready to merge.

After screenshots:

screen shot 4
screen shot 3
screen shot 2
screen shot 1
screen shot

@ebinnion ebinnion added [Type] Question [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. NUX People Management labels Dec 4, 2015
@ebinnion ebinnion self-assigned this Dec 4, 2015
@ebinnion ebinnion added this to the People Management: m6 milestone Dec 4, 2015
/**
* External dependencies
*/
import React from 'react';;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra semicolon here. Will remove as I continue to work on this.

@rickybanister
Copy link

This seems like a nice smart refactor.

The only feedback I'd have is to change Log Me In to Log In

At this point Log In is so ubiquitous that it looks like a picture, not two words. Throwing the 'me' in the middle made it pretty funky to read (I know that's weird, but trust me)

@ebinnion
Copy link
Contributor Author

ebinnion commented Dec 7, 2015

I'm going to ahead and cc @mtias for thoughts on factoring this out into a more global component.

@ebinnion
Copy link
Contributor Author

Closing in favor of #1521.

@ebinnion ebinnion closed this Dec 11, 2015
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 11, 2015
@lancewillett lancewillett deleted the try/logged-out-form-container branch December 22, 2015 19:13
@lancewillett lancewillett removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. NUX People Management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants