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

Add section login field above Oauth links #17276

Merged
merged 11 commits into from Aug 25, 2017
Merged

Conversation

ewjordan
Copy link
Contributor

@ewjordan ewjordan commented Aug 24, 2017

Also adds "OR" bar on the left side of the links section when on the sign in page (excluded from the new registration page):

(Updated with $light_gray instead of $black):

updated screenshot

(In-context, old color):

screen shot 2017-08-24 at 12 05 35 pm

@ewjordan
Copy link
Contributor Author

Working on UI tests now

%strong= Rails.env
%a{href: 'http://wiki.code.org/display/PROD/Developer+Oauth+setup'}
check the wiki.
- if devise_mapping.lockable? && resource_class.unlock_strategy_enabled?(:email) && controller_name != 'unlocks'
Copy link
Member

Choose a reason for hiding this comment

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

Is there a controller we could hoist such complex code out into?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unedited in this PR, from the original file which I think was brought over from https://github.com/plataformatec/devise/blob/master/app/views/devise/shared/_links.html.erb - I'm not entirely against pulling things out, but I'm a bit hesitant to do it here because I don't really know what that code is doing or how to properly test it.

.vertical_bar {
width: 2px;
height: 100%;
background-color: $black;
Copy link
Member

Choose a reason for hiding this comment

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

The screenshot looks like it's pure black, too, but we rarely use pure black for such a piece of UI. Probably $dark_charcoal will be the colour to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

As discussed online, probably lighter grey will work better. We can interate.

= link_to t('auth.need_unlock'), new_unlock_path(resource_name)
%br/
%br/
%div{style: "display: block;"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff got mangled here, what's really happening is everything that was previously here is put inside an enclosing div without edits (everything down to line 71 is essentially untouched apart from indentation).

.devise_link_section {
display: flex;
flex-direction: row;
justify-content: flex-start;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I mentioned before, but some of the earliest browsers we support have some limitations on their flexbox support and we should test explicitly. I think @islemaster knows the most about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've found http://caniuse.com/#feat=flexbox to be fairly accurate. Only IE11 is likely to cause issues for us, only in the following scenarios:

  • IE 11 requires a unit to be added to the third argument, the flex-basis property
  • IE 11 does not vertically align items correctly when min-height is used
  • In IE10 and IE11, containers with display: flex and flex-direction: column will not properly calculate their flexed childrens' sizes if the container has min-height but no explicit height property.

So generally flex-direction: column and min-height together is a red flag. I don't see that here, so this is likely to work fine. Worth testing though 👍.

And I type "9999999999" into "#section_code"
And I click selector "#section_form .btn-primary"
Then I wait until I am on "http://studio.code.org/courses"
And I wait for 10 seconds
Copy link
Member

Choose a reason for hiding this comment

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

Can we replace this timed wait for a wait for a specific element to be visible instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that was for local testing only as I was developing the test. My bad, good catch!

@ewjordan ewjordan merged commit e24ceb9 into staging Aug 25, 2017
@ewjordan ewjordan deleted the join-section-from-sign-in branch August 25, 2017 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants