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

Add Reader screen for unified prologue carousel #16185

Merged
merged 6 commits into from
Mar 30, 2021

Conversation

frosty
Copy link
Contributor

@frosty frosty commented Mar 29, 2021

Ref #15056. This PR adds the Reader screen to the prologue carousel.

iPhone iPad
Simulator Screen Shot - iPhone 12 Pro - 2021-03-29 at 21 53 38 Simulator Screen Shot - iPad Pro (9 7-inch) - 2021-03-29 at 21 55 08

To test

  • Build, run and log out if necessary
  • Verify that the Reader (last) page of the login prologue is consistent with the designs (see issue Prologue Carousel - Project Issue #15056; see also example screenshot above)

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Mar 29, 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 29, 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.

Hey @frosty , I left one code comment and found a few layout issues:

tags are not very visible in dark mode, looks like the background color did not switch:

on some devices, the vertical alignment is too high or too low (see examples). We might consider positioning the container view in UnifiedProloguePages relative to the screen size. I tried something like this in #16187

iPad Pro 9.7 iOS 13.5 iPhone SE 2nd gen iOS 14.4


/// A view modifier that applies style to a text to make it look like a tag token.
///
private struct TagItem: ViewModifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@frosty
Copy link
Contributor Author

frosty commented Mar 30, 2021

@Gio2018 I've fixed the dark mode issue, but let's tackle the spacing of all the views altogether in a future PR.

@frosty frosty requested a review from Gio2018 March 30, 2021 14:17
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.

Works great in dark mode.

A few optional minor adjustments to the icons might be made based on the designs, and yes let's address the positioning separately.

Base automatically changed from feature/swiftui-notifications-prologue-screen to develop March 30, 2021 20:50
@frosty frosty merged commit ec52b6f into develop Mar 30, 2021
@frosty frosty deleted the feature/swiftui-reader-prologue-screen branch March 30, 2021 20:50
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