mozilla-mobile / firefox-ios

Firefox for iOS
Mozilla Public License 2.0
12.1k stars 2.88k forks source link

Update Fonts related to Password / Passcode to use FXFontStyles #19642

Open data-sync-user opened 3 months ago

data-sync-user commented 3 months ago

This task is part of a series of tasks to standardizing fonts to start using FXFontStyles.

Background Context: Previously, we were using DefaultDynamicFontHelper and setting the text style, size and weight for each font, which sometimes did not match our design system. By using FXFontStyles, we can standardize our fonts and be more aligned with the design system.

Task: Please update how we set fonts related:

PasswordManagerTableViewCell DevicePasscodeRequiredViewController PasswordManagerOnboardingViewController CredentialPasscodeRequirementViewController

Acceptance Criteria:

Replace usage of DefaultDynamicFontHelper with FXFontStyles

If there are discrepancies in terms of replacing the old fonts with the new standard, we will need to bring design in to approve the PR.

Please provide before and after screenshots of the UI.

Reference:

Usage example: FXFontStyles.Regular.headline.scaledFont()

See this PR for example of standardizing fonts for Primary Button:

https://github.com/mozilla-mobile/firefox-ios/pull/18711/files

┆Issue is synchronized with this Jira Task

tisumi99 commented 3 months ago

I will work on this.

tisumi99 commented 3 months ago

Screenshot 2024-04-20 at 4 39 21 PM PasswordManagerTableViewCell.usernameLabel was orginally set to size 14, which is not defined in FXFontStyles. Used FXFontStyles.Regular.subheadline which has a size of 15.

Screenshot 2024-04-20 at 4 40 10 PM DevicePasscodeRequiredViewController.learnMoreButton was originally set to size 19, which is not defined in FXFontStyles. Used FXFontStyles.Regular.title3 which has a size of 20.

Screenshot 2024-04-20 at 4 40 53 PM PasswordManagerOnboardingViewController.onboardingMessageLabel was originally set to size 19, which is not defined in FXFontStyles. Used FXFontStyles.Regular.title3 which has a size of 20.

PasswordManagerOnboardingViewController.learnMoreButton font is actually set in the LinkButton type. Any font set directly to learnMoreButton was getting overwritten by LinkButton.init, configuration = UIButton.Configuration.plain(). Removed DefaultDynamicFontHelper from learnMoreButton and did not replace with FXFontStyles.

Screenshot 2024-04-20 at 4 41 13 PM CredentialPasscodeRequirementViewController.warningLabel was orginally set to size 18, which is not defined in FXFontStyles. Used FXFontStyles.Regular.body which has a size of 17.

cyndichin commented 3 months ago

@tisumi99 Thank you for the detailed screenshots and the breakdown of your decisions! @cwzilla can you verify these changes from design side?

cwzilla commented 3 months ago

Hi @tisumi99, thank you for the awesome screenshots! Just have a few requested changes and questions:

Thank you!

cyndichin commented 3 months ago

@cwzilla Regarding the learn more button, that is using our LinkButton component. The default value is using regular callout style. We can provide a separate font if needed via the view model.

The centering of the blue rounded button is separate, but @tisumi99 let me know if this is something you want to tackle while we are here. Otherwise, I can create a new ticket. Let me know what works best for you!

tisumi99 commented 3 months ago

I'll make the changes on PasswordManagerOnboardingViewController.onboardingMessageLabel and DevicePasscodeRequiredViewController.warningLabel to callout.

I'll take a look at centering the blue rounded button on this ticket.

There is an inconsitency on how learn more is styled on DevicePasscodeRequiredViewController.learnMoreButton versus PasswordManagerOnboardingViewController.learnMoreButton.

DevicePasscodeRequiredViewController.learnMoreButton is typed as UIButton and the font set to title3, size 20.
PasswordManagerOnboardingViewController.learnMoreButton. is typed as LinkButton.

Should I change DevicePasscodeRequiredViewController.learnMoreButton from UIButton to LinkButton?

cyndichin commented 3 months ago

thanks @tisumi99!

Should I change DevicePasscodeRequiredViewController.learnMoreButton from UIButton to LinkButton?

Yes if you don't mind, it's out of the scope of this ticket, but will help us standardize our components. The styling seems to differ so I'll default to @cwzilla to confirm.

tisumi99 commented 3 months ago

Updated the labels to callout. Centered the text in blue button. Learn More buttons consistently styled to LinkButton component.

Screenshot 2024-04-23 at 11 59 46 AM Screenshot 2024-04-23 at 12 00 08 PM

cwzilla commented 3 months ago

Updated the labels to callout. Centered the text in blue button. Learn More buttons consistently styled to LinkButton component.

Screenshot 2024-04-23 at 11 59 46 AM Screenshot 2024-04-23 at 12 00 08 PM

Changes look great, thanks for updating!!

cwzilla commented 3 months ago

thanks @tisumi99!

Should I change DevicePasscodeRequiredViewController.learnMoreButton from UIButton to LinkButton?

Yes if you don't mind, it's out of the scope of this ticket, but will help us standardize our components. The styling seems to differ so I'll default to @cwzilla to confirm.

I'm not sure I know what the difference is between UIButton and LinkButton.

cyndichin commented 3 months ago

thanks @tisumi99!

Should I change DevicePasscodeRequiredViewController.learnMoreButton from UIButton to LinkButton?

Yes if you don't mind, it's out of the scope of this ticket, but will help us standardize our components. The styling seems to differ so I'll default to @cwzilla to confirm.

I'm not sure I know what the difference is between UIButton and LinkButton.

Oh I meant to confirm the screenshots (since the UIButton is slightly bigger and lighter), but it seems based on your approval that LinkButton looks good as well!

cyndichin commented 3 months ago

LinkButton is our new component for the blue links. We were using UIButton (apple's default system button) previously, but since we have several links in the app with the same style we made a component of it. Hope that clarifies.

cwzilla commented 3 months ago

Oh gotcha. Yes makes sense, thanks for clarifying :) In that case, LinkedButton looks good!

data-sync-user commented 3 months ago

➤ Cyndi Chin commented:

Please review screenshots in the Github issue and validate that fonts scale appropriately. Thank you!

data-sync-user commented 2 months ago

➤ Diana Andreea Barladeanu commented:

Validated on v127 (41813), with iPhone 15 (17.4).