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

Unified About: Remove dependencies #17510

Merged
merged 4 commits into from Nov 22, 2021
Merged

Conversation

frosty
Copy link
Contributor

@frosty frosty commented Nov 19, 2021

Refs #17498. This PR is part of the work required to extract the new About screen out into a reusable Swift package. It makes the app header information (title, version, and icon) and font selection configurable by the host app.

Screenshot 2021-11-19 at 14 45 40

To test

  • Build and run and ensure that the header in the about screen looks as expected
  • Try changing the AppInfo and Fonts defined in WordPressAboutScreenConfiguration and check that your changes are reflected in the about screen. For example, you could change the fonts to:
static let fonts = AboutScreenFonts(appName: UIFont(name: "Zapfino", size: 28.0)!, appVersion: UIFont(name: "Chalkboard SE", size: 22.0)!)

to see the styles shown in the screenshot above.

Regression Notes

  1. Potential unintended areas of impact

The appearance of the header

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

I ensured that the default styles match what we had previously.

  1. What automated tests I added (or what prevented me from doing so)

N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@frosty frosty added this to the 18.8 milestone Nov 19, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 19, 2021

You can trigger an installable build for these changes by visiting CircleCI here.


static let defaultFonts: AboutScreenFonts = {
// Title is serif semibold large title
let fontDescriptor = UIFontDescriptor.preferredFontDescriptor(withTextStyle: .largeTitle)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love all this font wrangling here, but it's a tricky font to construct and I don't think we want to necessarily go down the route of adding UIFont extensions to this package?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 19, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

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

just a small comment that can be safely ignored, other than that :shipit: !

@frosty frosty merged commit 69ca695 into develop Nov 22, 2021
@frosty frosty deleted the feature/about-remove-dependencies branch November 22, 2021 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants