Bugfix FXIOS-13730 [New first run onboarding][iPad] The Skip button is missing from the onboarding on iPad #29833
Conversation
🥇 Perfect PR sizeSmaller PRs are easier to review. Thanks for making life easy for reviewers! ✨ 💬 Description craftsmanGreat PR description! Reviewers salute you 🫡 ❌ Per-file test coverage gateThe following changed file(s) are below 35.0% coverage:
Generated by 🚫 Danger Swift against 821742b |
cyndichin
left a comment
cyndichin
left a comment
There was a problem hiding this comment.
thanks @razvanlitianu had some comments during my initial review, thank you!
| Text(viewModel.skipText) | ||
| .bold() | ||
| .foregroundColor(skipTextColor) | ||
| .dynamicTypeSize(...DynamicTypeSize.xxxLarge) |
There was a problem hiding this comment.
Should we be using our FXFontStyles from the design system to adjust the this appropriately? I see you made scaledSwiftUIFont with a size cap, so curious on what this is doing here.
There was a problem hiding this comment.
Hey @cyndichin so this is not a font, it says basically "grow until the DynamicTypeSize.xxxLarge size but not beyond it". It is related to accessibility sizes and the last ones are really large.
| static let imageHeight: CGFloat = 150 | ||
| static let tosImageHeight: CGFloat = 70 | ||
| static let cornerRadius: CGFloat = 20 | ||
| static let secondaryButtonTopPadding: CGFloat = 8 |
| private func skipOnboarding() { | ||
| let currentIndex = min(max(viewModel.pageCount, 0), viewModel.onboardingCards.count - 1) | ||
| let currentCardName = viewModel.onboardingCards[currentIndex].name | ||
| viewModel.onComplete(currentCardName) | ||
| } |
There was a problem hiding this comment.
this seems like a good thing to test, curious why this live in the view instead of viewModel since logic looks like it can be self contained and the button action can be called viewModel.skipOnboarding
There was a problem hiding this comment.
Good catch. I'll change it.
| Button(" ", action: {}) | ||
| .font(UX.CardView.secondaryActionFont) | ||
| .buttonStyle(.borderedProminent) | ||
| .controlSize(.large) | ||
| .opacity(0) | ||
| .accessibilityHidden(true) | ||
| .disabled(true) |
There was a problem hiding this comment.
can we add a comment on what this empty button with no action is for? 🤔
|
@mergify rebase |
…s missing from the onboarding on iPad
razvanlitianu
force-pushed
the
rlitianu/FXIOS-13730-#29785-New-first-run-onboarding]iPad]-The-Skip-button-is-missing-from-the-onboarding-on-iPad
branch
from
7598d38 to
18d9a21
Compare
✅ Branch has been successfully rebased |
… identifiers (#29834) * Add correct key and value for Accessibility identifiers for shake to summarize * Remove unused label, add correct string key * Refactor strings * Add old string to proper struct * Add strings to 144 struct * Added missing strings to oldStruct * Restore previous key --------- Co-authored-by: Filippo Zazzeroni <[email protected]>
| } | ||
| } | ||
|
|
||
| func errorButtonLabel(for configuration: SummarizeViewConfiguration) -> String { |
There was a problem hiding this comment.
I see a bunch of changes related to summarizer, are these changes intentional?
| } | ||
| } | ||
|
|
||
| func errorButtonLabel(for configuration: SummarizeViewConfiguration) -> String { |
There was a problem hiding this comment.
I see a bunch of changes related to summarizer, are these changes intentional? cc: @issammani
📜 Tickets
Jira ticket
Github issue
💡 Description
Ensures the "Skip" button is visible during the first run onboarding flow on iPad, addressing its previous absence and improving user navigation for new users on iPad devices.
Navigate to firefox-ios/nimbus-features/onboardingFrameworkFeature.yaml
Change enableModernUi: false to enableModernUi: true
This enables the new SwiftUI-based onboarding framework
Before
After
🎥 Demos
Demo
📝 Checklist