-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Site Design Revamp] Recommends designs based on chosen vertical #16510
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
[Site Design Revamp] Recommends designs based on chosen vertical #16510
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APKs: |
|
Leaving this in draft state till it can be tested with the backend |
I've tested it by merging locally the pending changes to the backend and it works as expected: p1652300714807109-slack-C03CVCCN3J5 I guess we need the changes to be merged on the backend before actually merging this |
…ertical-designs # Conflicts: # build.gradle
…randomise-designs-order
|
Thank you for testing @ovitrif 🙇
To fully test this PR the |
| fun onChooseTapped() { | ||
| // TODO: adapt this to the new flow | ||
| selectedLayout?.let { layout -> | ||
| super.onPreviewChooseTapped() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkevins Delaying the super call that dismisses the preview screen and unselects the layout seems to fix the flow.
I removed the TODO but I'll be happy to put it back if there is more to it 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I added this comment before removing the bottom action buttons, and I left it in case anything else would need to be wired up when adding the new behavior. Delaying it sounds good to me (an alternative might be to pass it as a parameter, but I think this is simpler 🤷♂️ ).
One other consideration I had at the time was with regard to the tracks events, and whether we are differentiating between the user choosing from the main screen and the preview screen (in the old flow). In the new flow there is only one path to choose a theme, so maybe we just need to update the tracks description to reflect this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback @mkevins 🙇
One other consideration I had at the time was with regard to the tracks events, and whether we are differentiating between the user choosing from the main screen and the preview screen (in the old flow). In the new flow there is only one path to choose a theme, so maybe we just need to update the tracks description to reflect this?
That's a good point. I think with the old flow we do not differentiate between choosing directly or choosing in the preview screen and emit the same event enhanced_site_creation_site_design_selected. Despite that I think it's a good idea to make clear in the description of what we track when we update it to add the new recommended property.
ps. I added a draft issue [Site Design Revamp] Update Tracks event (non-blocking) on the board (cc @twstokes)
…randomise-designs-order
…track-recommended-property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @antonis, thank you for tackling this 🙇
✅ The code LGTM
✅ Following the test steps results works as described
🔴 I might have found a bug while doing exploratory testing:
- Tap back, pick another vertical → the recommended section doesn't update:
It's the same when using the skip button. It might be something related to how it works when the ViewModel already exists in memory.
Not sure if we want to fix it before merging to the feature branch or afterwards 🤷♂️
…designs-order [Site Design Revamp] Randomize order of designs for categories
|
Thank you for testing @ovitrif 🙇
|
Let's merge this since the issue can be tackled separately. Going to review the other PR that depends on this: #16538 |
…mmended-property [Site Design Revamp] Send recommended property with Tracks event on design selection
ovitrif
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #16503
Description
This PR determines recommended designs based on chosen vertical according to the following logic:
Note:
The randomisation of the recommended blog category order will be handled separately
To test:
To fully test this PR the
groupfield (refD80005) and recommended designs (refD80579) should be available on the backend or sandbox.Vertical with recommendations selected
Vertical selection skipped
Vertical has no recommendations
Regression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
Relied on existing test
What automated tests I added (or what prevented me from doing so)
Added tests for the recommendation logic in
SiteDesignRecommendationProviderTestPR submission checklist:
RELEASE-NOTES.txtif necessary.