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

Simplify /auth page #2295

Merged
merged 5 commits into from Nov 9, 2018

Conversation

Projects
None yet
3 participants
@dracos
Copy link
Member

dracos commented Oct 16, 2018

See sketch on ticket for what it should be. Fixes #2208. Splits up sign in/create/forgotten password to separate views (note we think this flow is less used than e.g. new report flow), with email/password question first.

  • Needs some design work to make sign in button look like email, tidy up heading run-on, probably something else I've missed.

@dracos dracos requested a review from zarino Oct 16, 2018

@wafflebot wafflebot bot added the Reviewing label Oct 16, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 16, 2018

Codecov Report

Merging #2295 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2295      +/-   ##
==========================================
- Coverage   80.42%   80.39%   -0.03%     
==========================================
  Files         187      187              
  Lines       12133    12141       +8     
  Branches     2241     2242       +1     
==========================================
+ Hits         9758     9761       +3     
- Misses       1616     1622       +6     
+ Partials      759      758       -1
Impacted Files Coverage Δ
perllib/FixMyStreet/App/Controller/Auth.pm 79.38% <100%> (+0.29%) ⬆️
perllib/FixMyStreet/TestMech.pm 91.08% <0%> (-1.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e1e2af...6c2fa7f. Read the comment docs.

@dracos dracos force-pushed the 2208-auth-page-changes branch from 3c62afd to 3411d4b Oct 16, 2018

@dracos dracos requested a review from davea Oct 16, 2018

@davea

This comment has been minimized.

Copy link
Member

davea commented Oct 16, 2018

A few bits I've spotted:

  • Would be nice to focus the password field if the user clicks the "sign in with password" button or hits enter (or tab!) in the email field:
    signin

Should we ask for the new password twice when creating an account?
image
(nb we already do this when changing your password:
image
)

This has sadly turned FMS into one of those annoying sites whose login forms break 1Password's form submission:
1password

@dracos

This comment has been minimized.

Copy link
Member

dracos commented Oct 16, 2018

"Should we ask for the new password twice when creating an account?" - I dunno, I don't think anyone does this, so I'd probably prefer stopping doing it on change password (perhaps paired with a 'show password' various sites have e.g. BBC).

"This has sadly turned FMS into one of those annoying sites whose login forms break 1Password's form submission" - gah, I specifically tried this (I use it too) and it works for me - but I don't have auto-submit turned on. So it filled in email, and when I clicked "Sign in with password" it had pre-filled password. So I guess I just need to figure out how it can find the submit...

@dracos

This comment has been minimized.

Copy link
Member

dracos commented Oct 17, 2018

It should now work on enter in email box, focus on password box, and work with (at least 1password) autosubmit (it shows the form quickly on password change, which appears to be enough here anyway).

@dracos dracos force-pushed the master branch from e6b5590 to b5f57d1 Oct 22, 2018

@dracos dracos added the Design label Oct 23, 2018

@dracos dracos force-pushed the 2208-auth-page-changes branch from bf35a9e to 598fe9b Oct 23, 2018

@davea
Copy link
Member

davea left a comment

One minor stylistic issue, sorry. Apart from that, this is a much nicer sign-in experience!

Show resolved Hide resolved templates/web/base/auth/general.html Outdated

@dracos dracos force-pushed the 2208-auth-page-changes branch 2 times, most recently from f86617c to 104b66d Oct 26, 2018

@wafflebot wafflebot bot added the Reviewing label Nov 7, 2018

@zarino

zarino approved these changes Nov 7, 2018

Copy link
Member

zarino left a comment

This is all fantastic!

I’ve added a few commits to fix up the styling. It included a fairly kludgey .btn--block ruleset with a boat load of !important flags, which makes me feel a little bit sick inside, but I couldn’t think of a simpler way to do it 😞

@dracos dracos force-pushed the 2208-auth-page-changes branch 2 times, most recently from b447667 to 654aaaf Nov 7, 2018

Show resolved Hide resolved web/cobrands/sass/_base.scss Outdated

@zarino zarino force-pushed the 2208-auth-page-changes branch from 9ee909a to e6c6861 Nov 9, 2018

@dracos
Copy link
Member

dracos left a comment

Is btn btn--primary meant to be the same as green-btn? The comment implies not, but e.g. Lincs and fixmystreet.com need a btn--primary adding if that's correct.

Greenwich can also have input/button specific removed, I think.

Show resolved Hide resolved web/cobrands/oxfordshire/base.scss Outdated
Show resolved Hide resolved web/cobrands/sass/_base.scss Outdated
Show resolved Hide resolved web/cobrands/sass/_base.scss Outdated

@zarino zarino force-pushed the 2208-auth-page-changes branch 2 times, most recently from 502f6b3 to e875541 Nov 9, 2018

@dracos
Copy link
Member

dracos left a comment

Two more input.green-btn to remove; the commit that removes btn--social calls it btn--small in the commit message. Think that's it!

Show resolved Hide resolved web/cobrands/oxfordshire/base.scss Outdated
Show resolved Hide resolved web/cobrands/oxfordshire/base.scss Outdated

@zarino zarino force-pushed the 2208-auth-page-changes branch from 4cdae37 to 045a551 Nov 9, 2018

dracos and others added some commits Oct 15, 2018

Make .btn--block work on button and input elements
The intention of .btn--block is to make the element full width. Because
of the weird way browsers handle sizing of form elements, just setting
`display: block` on `<button>` and `<input type="submit">` elements
wasn’t making them full width. Instead, .btn--block needed to explicitly
set a 100% width, and then reset any margins or box-sizing issues that
might cause it to overflow its parent.
Remove a few .btn related unnecessary things.
.btn--social doesn't do anything, and neither do the text-transforms.
Tidy up UK council cobrand button style overrides
Given recent changes to the button mixins, now is a good opportunity to
clear out some overcomplicated button styling from a few UK council
cobrands: Greenwich, Hart, and Oxfordshire.

All three referred to `button.green-btn` elements that don’t seem to
exist any more, and I couldn’t find a reason for the extra specificity
in the `input.green-btn` selectors either.

Oxfordshire’s buttons are drastically restyled from the FMS defaults,
so its button rules are a little complex. But I’ve tried to make the
distinction clearer between buttons we’re having to build from scratch
(.btn, .green-btn, etc) and buttons we’re simply re-colouring.

@dracos dracos force-pushed the 2208-auth-page-changes branch from 045a551 to 6c2fa7f Nov 9, 2018

@dracos dracos merged commit 6c2fa7f into master Nov 9, 2018

4 checks passed

codecov/patch 100% of diff hit (target 80.42%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +19.57% compared to 7e1e2af
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wafflebot wafflebot bot removed the Reviewing label Nov 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment