Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Refactor + improve test coverage for QR login #9525

Merged
merged 3 commits into from
Nov 2, 2022
Merged

Conversation

hughns
Copy link
Member

@hughns hughns commented Oct 31, 2022

Resolves element-hq/element-web#23549

This PR includes a refactor to provide greater separation between UI and logic.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@hughns hughns added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Oct 31, 2022
@hughns hughns force-pushed the hughns/qr-login-tests branch from c4664d8 to bb60dd1 Compare October 31, 2022 23:04
@hughns hughns marked this pull request as ready for review November 1, 2022 09:25
@hughns hughns requested a review from a team as a code owner November 1, 2022 09:25
Copy link
Contributor

@duxovni duxovni left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

cancellationMessage = _t("An unexpected error occurred.");
break;
case RendezvousFailureReason.HomeserverLacksSupport:
cancellationMessage = _t("The homeserver doesn't support signing in another device.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cancellationMessage = _t("The homeserver doesn't support signing in another device.");
cancellationMessage = _t("The homeserver doesn't support using a device to sign in another device.");

(As written, it sounds like you're not allowed to have more devices on your account at all.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I've fed this into a design review which is in-flight. 👍

@hughns hughns merged commit f05cc5d into develop Nov 2, 2022
@hughns hughns deleted the hughns/qr-login-tests branch November 2, 2022 10:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test coverage to QR code login
2 participants