-
Notifications
You must be signed in to change notification settings - Fork 581
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
feat(ONYX-1457): add Create Alert Prompt experiment #11346
base: main
Are you sure you want to change the base?
feat(ONYX-1457): add Create Alert Prompt experiment #11346
Conversation
41f0661
to
ededf25
Compare
/** | ||
* if Create Alert CTA was pressed withing 2 minutes after the prompt was shown | ||
* do not show the prompt again | ||
* */ | ||
if (Date.now() - promptState.dismissDate < 120000 && isFocused) { | ||
dontShowCreateAlertPromptAgain() | ||
} |
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.
This part is still TDB whether to keep it or not
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.
Barney confirmed this is a good enough approach - if the user interacted with the Create Alert button within 2 minutes after seeing the message or a popover we should not show the message or popover again
const isOnboardingFinished = | ||
isFocused && | ||
// make sure threre is no active onboarding popover on the screen | ||
!activePopover |
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.
We should not display two popovers on the same screen at the same time, tis is why I am adding this check here.
From my understanding, this is the easiest ways to check if there is an active popover from the progressive onboarding. Not sure if this is the most reliable implementation.. Please, correct me if I am missing something
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.
Just noticed that isOnboardingFinished
is not a very descriptive name
Should be something like "canDisplayPopover"
const variant_a = enabled && variant === "variant-a" | ||
const variant_b = enabled && variant === "variant-b" |
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.
🐪 🐍
const variant_a = enabled && variant === "variant-a" | |
const variant_b = enabled && variant === "variant-b" | |
const variantA = enabled && variant === "variant-a" | |
const variantB = enabled && variant === "variant-b" |
const variant_a = enabled && variant === "variant-a" | ||
const variant_b = enabled && variant === "variant-b" | ||
|
||
const userDidScroll = artworks.length >= 40 |
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.
This might work at the moment, but it does not precisely determine whether the user has scrolled or not. I think there are three things here we could improve:
- Update the logic to either involve the scroll offset or check whether the number of artworks is greater than the initial number of artworks fetched (I'm not 100% sure if this is necessary, though).
- Avoid magic numbers.
- Change the variable name because the user could have already scrolled when the number of artworks is less than or equal to 40.
const userDidScroll = artworks.length >= 40 | |
const displayCreateAlertPrompt = artworks.length >= CREATE_ALERT_ONBOARDING_OFFSET && artworks.length > INITIAL_NUMBER_OF_ARTWORKS |
@@ -288,6 +304,7 @@ const ArtworksGrid: React.FC<ArtworksGridProps> = ({ | |||
<ArtistArtworksFilterHeader | |||
artist={artist} | |||
showCreateAlertModal={() => setIsCreateAlertModalVisible(true)} | |||
shouldShowCreateAlertPrompt={!!userDidScroll && variant_a} |
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.
Not sure if prompt (definition: "a request the software makes in order to obtain information from the end user...") is the correct term here. Maybe onboarding tooltip makes more sense 🤔
@@ -9,11 +9,13 @@ import { graphql, useFragment } from "react-relay" | |||
interface ArtistArtworksFilterProps { | |||
artist: ArtistArtworksFilterHeader_artist$key | |||
showCreateAlertModal: () => void | |||
shouldShowCreateAlertPrompt?: boolean |
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.
I wonder if we can avoid prop drilling here by using the already existing ProgressiveOnboardingModel
's setIsReady functionality.
@@ -0,0 +1,33 @@ | |||
import { action, Action } from "easy-peasy" | |||
|
|||
export interface CreateAlertPromptModel { |
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.
Can we use the already existing ProgressiveOnboardingModel
for this feature?
@@ -57,6 +58,7 @@ interface GlobalStoreStateModel { | |||
requestedPriceEstimates: RequestedPriceEstimatesModel | |||
recentPriceRanges: RecentPriceRangesModel | |||
progressiveOnboarding: ProgressiveOnboardingModel | |||
createAlertPrompt: CreateAlertPromptModel |
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.
models should be ideally names instead of verbs. Maybe something like alertPrompt
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.
However, I don't think we should add a new model for this. It might be possible for us to use the existing visual clues model to model this.
We already have addClue
and setVisualClueAsSeen
. Maybe we can find a way to save the number of times an item has been seen and combine both models into one
This PR resolves ONYX-1457
Description
Help increase awareness of the Create Alert function from the artist page (high traffic and high impact)
Figma
a
andb
the component should be shown:Also adding the
payloadSuggestions: ['{"forcePrompt": "true"}', '{"forcePrompt": "false"}']
for testing purposes. Inspired by @anandaroop 🙏When
forcePrompt === true
the max times to show the component is 123456 and the waiting time is 30 seconds instead of 3 days. Also, in case of a forced prompt I display a text that says how many times the component has been shown. If the Create Alert CTA was not pressed after seeing the component, on the next render of the component the number will increase by 1. If Create Alert CTA was pressed, the number will be increased by 2. This functionality was added for testing purposes.iOS
popul.Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-01-08.at.18.38.55.mp4
message.Simulator.Screen.Recording.-.iPhone.16.Pro.Max.-.2025-01-08.at.18.38.18.mp4
Android
IMG_7221.MP4
IMG_7220.MP4
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.