Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[MBL-1162] Fix login flow styling #1925

Merged
merged 16 commits into from
Feb 6, 2024
Merged

[MBL-1162] Fix login flow styling #1925

merged 16 commits into from
Feb 6, 2024

Conversation

ifosli
Copy link
Contributor

@ifosli ifosli commented Jan 31, 2024

📲 What

Update login flow classes to use self.traitCollection.userInterfaceIdiom whenever we want to find out if we should use iPad styling or not. The choice of styling here came from me sending Alison a few screenshots and getting her opinion.

Note: I've also updated tests to significantly reduce the number of snapshots we need to deal with; instead of checking all possible combinations, we create a minimal set of combinations where each option is included.

Note: This pr does NOT get rid of every instance of subview.traitCollection; there are too many :(

🤔 Why

Styling in the login flow has gotten weird in iOS 17 for a few reasons. First, traitCollections don't always propagate correctly from viewcontrollers to subviews (though they sometimes seem to?). This means that we should use self.traitCollection instead, particularly for tests. Second, the login flow is presented using a modal overlay, which means that the view itself has a compact horizontal size class. This means that we should check that the idiom is .pad instead of checking the size classes, since we do still want different styling on iPad.

Note: I kept the two factor verification screen in terms of size classes instead, since it got so compressed that the english sentence got truncated at the default font otherwise.

👀 See

JIRA

Before 🐛 After 🦋
image image
image image
image image
image image
image image
image image
image image

♿️ Accessibility

Not perfect, but also not worse. Log in button is hidden (scroll down to see it) for larger font sizes.

✅ Acceptance criteria

  • Tests are representative of app
  • Login flow looks decent

@ifosli ifosli self-assigned this Jan 31, 2024
@ifosli ifosli added this to the release 5.13.0 milestone Jan 31, 2024
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

Looks good. I'll admit that I don't fully understand how those orthogonal methods work exactly yet though

@ifosli
Copy link
Contributor Author

ifosli commented Feb 1, 2024

Looks good. I'll admit that I don't fully understand how those orthogonal methods work exactly yet though

Let's say we want to test [a, b] traits from one group (ex languages) and [x, y, z] traits from another group (ex devices). Our current combos will test every combination; ie [(a, x), (a, y), (a, z), (b, x), (b, y), (b, z)] for a total of six screenshots. The orthogonal combos ensures each trait is represented at least once, by combining them linearly; ie [(a,x), (b, y), (a,z)] for three screenshots. (It tests a twice because there aren't as many traits from the first group, so it starts looping through them again).

Most of our tests are run either 10 or 15 times (5 languages and 2 or 3 devices) - with this change, we'll only run them 5 times. And, if we have multiple similar tests within one class, we don't even need to test each one of those 5 times; once each is probably enough, as long as we vary the device and language.

@scottkicks
Copy link
Contributor

@ifosli got it! thanks for explaining!

@ifosli
Copy link
Contributor Author

ifosli commented Feb 6, 2024

Updating the spacing again with Alison's okay, including "after" screenshots. Spacing is final now!

@ifosli ifosli requested a review from scottkicks February 6, 2024 00:07
@ifosli ifosli merged commit 1c52fe4 into main Feb 6, 2024
5 checks passed
@ifosli ifosli deleted the loginFlowPadPadding branch February 6, 2024 17:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants