Skip to content

Login: fix Google button covering error label - #7902

Merged
nheagy merged 9 commits into
developfrom
fix/7898-google-button-covering
Oct 12, 2017
Merged

Login: fix Google button covering error label#7902
nheagy merged 9 commits into
developfrom
fix/7898-google-button-covering

Conversation

@nheagy

@nheagy nheagy commented Oct 3, 2017

Copy link
Copy Markdown
Contributor

Fixes #7898

simulator screen shot - iphone 8 - 2017-10-03 at 11 07 51

To test:

  • ensure the input field and labels still work as before
  • ensure the error label can't be covered by the Google button

Needs review: @aerych
Note: forked from #7887

@nheagy nheagy added the Login label Oct 3, 2017
@nheagy nheagy added this to the 8.6 milestone Oct 3, 2017
@nheagy nheagy self-assigned this Oct 3, 2017
@nheagy
nheagy requested a review from aerych October 3, 2017 17:09
@aerych

aerych commented Oct 3, 2017

Copy link
Copy Markdown
Contributor

Looks like there is some spacing issues on the SE when the keyboard is present.

Before:

simulator screen shot - iphone se - 2017-10-03 at 15 06 02

After:

simulator screen shot - iphone se - 2017-10-03 at 14 55 50

Notice how the info text is truncating where it wasn't before.

I also noticed the same thing you did, when the error screen is visible space is very tight.
simulator screen shot - iphone se - 2017-10-03 at 14 56 12

Localizations can make this worse. German is one example that comes to mind. I wonder if the error should replace the Google button in these cases. Not sure. Thoughts?

@nheagy

nheagy commented Oct 4, 2017

Copy link
Copy Markdown
Contributor Author

I wonder if the error should replace the Google button in these cases.

Ya, that's probably the right thing to do. @folletto any thoughts?

@folletto

folletto commented Oct 4, 2017

Copy link
Copy Markdown

I replied on Slack, reporting here too:

The mental model for that should be that the bottom "log into... Next" is on a separate floating block on top (z-index: 1 equivalent) so the "page" scrolls below
Another alternative would be to hide Google login message when this condition is true: keyboard visible && error message

@aerych

aerych commented Oct 4, 2017

Copy link
Copy Markdown
Contributor

Another alternative would be to hide Google login message when this condition is true: keyboard visible && error message

👍

@elibud elibud modified the milestones: 8.6, 8.7 Oct 9, 2017
@nheagy

nheagy commented Oct 10, 2017

Copy link
Copy Markdown
Contributor Author

Ok, @aerych this is ready for another look.

Here's how hiding the Google button looks, cc @folletto

7879-google-button-animation-flat

@aerych

aerych commented Oct 11, 2017

Copy link
Copy Markdown
Contributor

Small nitpick... could we leave the button visible when there is an external keyboard in use?

simulator screen shot - ipad pro 9 7-inch - 2017-10-11 at 07 25 28

@aerych

aerych commented Oct 11, 2017

Copy link
Copy Markdown
Contributor

By the way... the button transition when the keyboard hides/shows is very nice :)

@folletto

Copy link
Copy Markdown

Well done 👍

@nheagy
nheagy force-pushed the fix/7898-google-button-covering branch from 8ece3f2 to 586b475 Compare October 11, 2017 21:09
@nheagy

nheagy commented Oct 11, 2017

Copy link
Copy Markdown
Contributor Author

Fixed the appearance on iPad and the conflicts.

Assuming Hound is pleased, this should be ready for another loock @aerych

@aerych aerych left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice. :shipit: once podfile lock merge conflicts are sorted :)

@nheagy
nheagy merged commit b7d51f5 into develop Oct 12, 2017
@nheagy
nheagy deleted the fix/7898-google-button-covering branch October 12, 2017 20:50
@nheagy nheagy mentioned this pull request Oct 13, 2017
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants