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 prologue - Stats screen #16187

Merged
merged 11 commits into from
Mar 31, 2021
Merged

Unified prologue - Stats screen #16187

merged 11 commits into from
Mar 31, 2021

Conversation

Gio2018
Copy link
Contributor

@Gio2018 Gio2018 commented Mar 30, 2021

Ref #15056

NOTE: this PR calculates the distance between the prologue title and the content view relative to the actual view height, and removes the Spacer above and below the SwiftUI view. As a consequence, if we follow this approach, other prologue pages need to be updated

To test:

iPhone iPad

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

@Gio2018 Gio2018 added this to the 17.1 milestone Mar 30, 2021
@Gio2018 Gio2018 requested a review from frosty March 30, 2021 00:12
@Gio2018 Gio2018 self-assigned this Mar 30, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 30, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 30, 2021

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

Copy link
Contributor

@frosty frosty left a comment

Choose a reason for hiding this comment

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

This is looking good! If I was going to nitpick against the original designs I would say:

  • The purple face icon could be a little lower and perhaps a little bigger
  • The people icon could be a little bigger too
  • The chart element could be a little taller (the map may want to be a little less tall to account for this)

But these are all very minor.

  • Also, on iPhone 12 Pro this content view appears a little lower than the editor content view – I think because the title runs over 3 lines instead of 2. Again, this might be something we address separately.

@@ -75,9 +75,14 @@ class UnifiedProloguePageViewController: UIViewController {
private func configureContentView() {
contentView.translatesAutoresizingMaskIntoConstraints = false
view.addSubview(contentView)

let contentViewHeight: CGFloat
if let windowHeight = UIApplication.shared.mainWindow?.frame.height {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure about using the main window, as this isn't very future proof if we support multiple scenes / windows in the future? Also if the window size changes (for example rotating an iPad)?

@Gio2018
Copy link
Contributor Author

Gio2018 commented Mar 30, 2021

Thanks for reviewing @frosty

  • The purple face icon could be a little lower and perhaps a little bigger
  • The people icon could be a little bigger too
  • The chart element could be a little taller (the map may want to be a little less tall to account for this)

sizes and positions changed.

  • Also, on iPhone 12 Pro this content view appears a little lower than the editor content view – I think because the title runs over 3 lines instead of 2. Again, this might be something we address separately.

Agreed to fix these in a separate PR.

@Gio2018 Gio2018 requested a review from frosty March 30, 2021 15:23
Copy link
Contributor

@frosty frosty 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!

@Gio2018 Gio2018 merged commit ee2e6bd into develop Mar 31, 2021
@Gio2018 Gio2018 deleted the feature/prologue-stats-screen branch March 31, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants