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

Passwordless | Move sign in with password instead option #3058

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

coldlink
Copy link
Member

@coldlink coldlink commented Feb 7, 2025

What does this change?

Recently we deployed sign in with passcodes to all users.

While we noticed that overall sign in numbers remained consistent, we did get a few user complaints saying that they did not like how the "sign in with password" option was only available after getting the one time code sent to their email. Other users complained that they thought they could input a password into the passcode field box and sign in with a password directly.

To improve the UX, we've made some minor adjustments to the sign in flow.

Firstly we added a "sign in with a password instead" link to the sign in landing page, which takes the email from the email input, and redirects the user to /signin/password with the email option prefilled, or directly to the page if there is no email. We also moved the "sign in with a password instead" option on the passcode input page to the information box, as an additional option if a user is having trouble with passcodes.

We belive that this should alleviate some of the issues users are having, even as we're prioritising sign in with passcodes.

We also rename the passcode CTA from "Continue with email" to Sign in with email" instead.

Sign In page Passcode page
Screen.Recording.2025-02-10.at.15.11.03.mov
Screen.Recording.2025-02-10.at.15.11.25.mov

Tested

  • DEV
  • CODE

@coldlink coldlink requested review from a team and removed request for a team February 7, 2025 11:25
@coldlink coldlink force-pushed the mm/move-password-option branch from 1f4084a to d01ed27 Compare February 10, 2025 09:21
@coldlink coldlink requested review from guardian-ci and removed request for guardian-ci February 10, 2025 09:22
@coldlink coldlink added the passwordless PRs/Issues related to passwordless/passcode functionality label Feb 10, 2025
@coldlink coldlink force-pushed the mm/move-password-option branch from d01ed27 to 026e812 Compare February 10, 2025 09:58
@coldlink coldlink marked this pull request as ready for review February 10, 2025 10:05
@coldlink coldlink requested a review from a team as a code owner February 10, 2025 10:05
coldlink added a commit to guardian/manage-frontend that referenced this pull request Feb 10, 2025
Identity have changed the way that password sign in works in guardian/gateway#3058, so update the  test behaviour to reflect that.
@coldlink coldlink force-pushed the mm/move-password-option branch from 026e812 to 3ccc95f Compare February 10, 2025 14:49
@coldlink coldlink requested review from a team and removed request for a team February 10, 2025 14:53
@coldlink coldlink force-pushed the mm/move-password-option branch from 3ccc95f to 341ddac Compare February 10, 2025 15:00
@coldlink coldlink requested review from a team and removed request for a team February 10, 2025 15:02
@coldlink coldlink force-pushed the mm/move-password-option branch from 341ddac to b416b0f Compare February 10, 2025 15:15
@coldlink coldlink requested review from a team and removed request for a team February 10, 2025 15:15
@@ -89,12 +91,20 @@ export const EmailSentInformationBox = ({
)}
{changeEmailPage && (
<>
, or{' '}
,{!showSignInWithPasswordOption ? <> or </> : <> </>}
Copy link
Contributor

@pvighi pvighi Feb 10, 2025

Choose a reason for hiding this comment

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

sorry I'm having a bit of trouble understanding the intent (or maybe the syntax) here:

is the intention that if it has to show both links it would say
try another address or sign in with a password instead
but if it only has to one it would just say or try another address / or sign in with a password instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

the latter option!

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@pvighi pvighi Feb 10, 2025

Choose a reason for hiding this comment

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

is a comma before 'or' correct ?
It looks weird to me but sadly I don't have an english degree to know this kind of things for sure.

edit: I mean in cases like this:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

In generally I've always used a comma before the or in the last item in a list, so it separated from the other items in the same way, like an array.

See Oxford comma in the Guardian style guide: https://www.theguardian.com/guardian-observer-style-guide-o#:~:text=what%20we%20use-,Oxford%20comma,-a%20comma%20before

Comment on lines 155 to 164
useEffect(() => {
if (typeof window !== 'undefined' && focusPasswordField && email) {
const passwordInput: HTMLInputElement | null =
window.document.querySelector('input[name="password"]');

if (passwordInput) {
passwordInput.focus();
}
}
}, [focusPasswordField, email]);
Copy link
Member

@AshCorr AshCorr Feb 10, 2025

Choose a reason for hiding this comment

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

Would autofocus work here?

For some reason my screen reader isn't announcing the "password" field when its getting .focus()ed, not sure if thats a side effect from it being called by useEffect or if im just using the screen reader wrong, but I wonder if using the browsers built in autofocus it might work better. Its my Voiceover.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll refactor to use autofocus anyway!

As it avoids a useEffect and any client side code, as the autofocus attribute can be applied on the server side render of the page!

@coldlink coldlink requested review from a team and removed request for a team February 10, 2025 16:56
@coldlink coldlink force-pushed the mm/move-password-option branch from 3489cbd to 4123b82 Compare February 10, 2025 17:02
@coldlink coldlink requested a review from a team February 10, 2025 17:03
@coldlink coldlink merged commit c169773 into main Feb 11, 2025
21 checks passed
@coldlink coldlink deleted the mm/move-password-option branch February 11, 2025 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
passwordless PRs/Issues related to passwordless/passcode functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants