Bugfix FXIOS-7646 [v120] The 3rd CFR is not displayed #17172
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lmarceau
reviewed
Nov 6, 2023
Client/Frontend/Browser/BrowserViewController/BrowserViewController+URLBarDelegate.swift
Outdated
Show resolved
Hide resolved
| @@ -113,6 +113,7 @@ struct ContextualHintEligibilityUtility: ContextualHintEligibilityUtilityProtoco | |||
| if cfrCounter <= 2, !hasOptedIn, hasTimePassed { | |||
| // - Display CFR-1 | |||
| profile.prefs.setInt(cfrCounter + 1, forKey: PrefsKeys.ContextualHints.shoppingOnboardingCFRsCounterKey.rawValue) | |||
| profile.prefs.setTimestamp(Date.now(), forKey: PrefsKeys.FakespotLastCFRTimestamp) | |||
There was a problem hiding this comment.
Can the tests for canPresentShoppingCFR be adjusted to represent this new addition?
…ttps://github.com/PARAIPAN9/firefox-ios into sparaipan/fxios-7646-The-3rd-CFR-is-not-displayed
5 tasks
|
It was discovered that on smaller devices like the iPhone SE, the action button label would go to the next line, even though there was enough space on one line. After investigating, I found that the resizable button was causing layout problems. Therefore, I changed the button to a UIButton using configuration, and it fixed the issue.
|
thatswinnie
reviewed
Nov 9, 2023
BrowserKit/Sources/ComponentLibrary/ContextualHintView/ContextualHintView.swift
Show resolved
Hide resolved
|
Just wanna say LGTM from my side for the comments I made |
thatswinnie
reviewed
Nov 9, 2023
| @@ -75,6 +77,18 @@ public class ContextualHintView: UIView, ThemeApplicable { | |||
| public func configure(viewModel: ContextualHintViewModel) { | |||
| self.viewModel = viewModel | |||
|
|
|||
| let actionButtonViewModel = LinkButtonViewModel( | |||
| title: viewModel.actionButtonTitle, | |||
| a11yIdentifier: "", | |||
There was a problem hiding this comment.
Can we please add an a11y id here?
| let actionButtonViewModel = LinkButtonViewModel( | ||
| title: viewModel.actionButtonTitle, | ||
| a11yIdentifier: "", | ||
| contentInsets: NSDirectionalEdgeInsets( |
There was a problem hiding this comment.
The contentInsets should be defined in the UX struct.
thatswinnie
approved these changes
Nov 9, 2023
Client.app: Coverage: 32.6
WidgetKitExtension.appex: Coverage: 23.17
Generated by 🚫 Danger Swift against 5ece51e |
|
@Mergifyio backport release/v120 |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Nov 9, 2023
* Use one instance for the shopping CFR * Rename contextHintVC and add new unit test * Initialize shopping contextual hint in init of BVC * Fix crash that can happen when manually changing time on the device * Fix stackview layout by using UIButton configuration * Change UIButton to LinkButton * Remove configuration from actionButton * Add a11y id and move button insets to UX struct --------- Co-authored-by: Winnie Teichmann <[email protected]> (cherry picked from commit 7d43a40)
thatswinnie
pushed a commit
that referenced
this pull request
Nov 9, 2023
* Use one instance for the shopping CFR * Rename contextHintVC and add new unit test * Initialize shopping contextual hint in init of BVC * Fix crash that can happen when manually changing time on the device * Fix stackview layout by using UIButton configuration * Change UIButton to LinkButton * Remove configuration from actionButton * Add a11y id and move button insets to UX struct --------- Co-authored-by: Winnie Teichmann <[email protected]> (cherry picked from commit 7d43a40) Co-authored-by: PARAIPAN SORIN <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📜 Tickets
Jira ticket
Github issue
💡 Description
Changes in this PR:
I've noticed that the 'urlBarPresentCFR' is being called multiple times, leading to the creation of multiple 'contextualVCs.' This not only disrupts the logic but also causes memory leaks. Ensuring that we work with the same reference can help us avoid redundant calls to the 'canPresent' method in 'ContextualHintEligibilityUtility,' thereby preventing user prefs issues and memory leaks.
Additionally, after the second 'CFR' is displayed, I reset the timestamp. Otherwise, if 24 hours have passed, two 'CFRs' might appear consecutively.
📝 Checklist
You have to check all boxes before merging