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

Simplify the layout of the onboarding splash screen #6320

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Jun 20, 2022

Fixes #6319 by simplifying the layout of the splash screen:

  • The gradient is now separate from the carousel pages
  • The carousel pages are now in a VStack with the buttons rather than using a ZStack and ViewFrameReader.

The second commit in the PR updates the structure of the splash screen to match the other SwiftUI screens with the body above all of the other views. It is probably easiest to review this PR by commit because of this.

Edit: It turns out all of these changes didn't fix the issue, what did was increasing the maxWidth on the carousel images in the last commit. Tested with screens of 320pt, 375pt, 390pt, 414pt and 428pt and no stuttering noticed, but this feels like a rather fragile assumption 😕

iPod Touch iPhone SE (3rd gen) iPhone 13 Mini iPhone 13 Pro Max
Simulator Screen Shot - iPod touch (7th generation) - 2022-06-20 at 16 30 19 Simulator Screen Shot - iPhone SE (3rd generation) - 2022-06-20 at 16 30 57 Simulator Screen Shot - iPhone 13 mini - 2022-06-20 at 16 29 39 Simulator Screen Shot - iPhone 13 Pro Max - 2022-06-20 at 16 31 51
11" iPad Portrait 11" iPad Landscape
Simulator Screen Shot - iPad Pro (11-inch) (3rd generation) - 2022-06-20 at 16 33 46 Simulator Screen Shot - iPad Pro (11-inch) (3rd generation) - 2022-06-20 at 16 33 53

@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@pixlwave pixlwave marked this pull request as ready for review June 20, 2022 15:34
@pixlwave pixlwave requested review from a team, SBiOSoftWhare and aringenbach and removed request for a team June 20, 2022 15:35
@codecov-commenter
Copy link

Codecov Report

Merging #6320 (80e54b3) into develop (a8514ed) will increase coverage by 0.00%.
The diff coverage is 93.81%.

@@           Coverage Diff            @@
##           develop    #6320   +/-   ##
========================================
  Coverage     6.24%    6.24%           
========================================
  Files         1440     1440           
  Lines       155412   155419    +7     
  Branches     62532    62533    +1     
========================================
+ Hits          9705     9713    +8     
+ Misses      145302   145301    -1     
  Partials       405      405           
Impacted Files Coverage Δ
.../Modules/Onboarding/Common/OnboardingMetrics.swift 100.00% <ø> (+100.00%) ⬆️
...ing/SplashScreen/View/OnboardingSplashScreen.swift 58.66% <91.52%> (+1.92%) ⬆️
...SplashScreen/View/OnboardingSplashScreenPage.swift 81.25% <95.45%> (-3.20%) ⬇️
...ng/SplashScreen/OnboardingSplashScreenModels.swift 90.00% <100.00%> (+2.50%) ⬆️
...dules/Common/ViewFrameReader/ViewFrameReader.swift 0.00% <0.00%> (-100.00%) ⬇️
...ojiPicker/Data/EmojiMart/EmojiItem+EmojiMart.swift 95.65% <0.00%> (+0.30%) ⬆️
...UI/Modules/Common/Util/ReadableFrameModifier.swift 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8514ed...80e54b3. Read the comment docs.

@github-actions
Copy link

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/V99BWg

Copy link
Contributor

@aringenbach aringenbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I'm not sure we can have a better fix anyway (I was thinking about nesting the Image in a container and applying some of the frame modifiers to the container and see what happens, but I'm not sure how much effort we want to put into something that looks like a pure iOS/SwiftUI issue).

@pixlwave
Copy link
Member Author

Note: There's a second review on these changes on ElementX.

@pixlwave pixlwave merged commit 2d1f6f8 into develop Jun 22, 2022
@pixlwave pixlwave deleted the doug/6319-splash-screen branch June 22, 2022 08:54
stefanceriu pushed a commit that referenced this pull request Jun 28, 2022
* Simplify the layout of the onboarding splash screen
* Re-organise OnboardingSplashScreen.
* Fix frame drops for real this time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The splash screen animation can stutter on some devices.
3 participants