-
Notifications
You must be signed in to change notification settings - Fork 583
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-1293): redesign save cta and add follow cta #11062
feat(ONYX-1293): redesign save cta and add follow cta #11062
Conversation
Note: the tracking of the experiment has to be adjusted: we need to track the experiment on the ArtworksRail component and track once per session. Will be addressed in a separate PR |
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.
Nice one - I left a few comments. please let me know if something isn't clear
src/app/system/devTools/DevMenu/Components/ExperimentFlagItem.tsx
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,26 @@ | |||
export const getExperimentVariant = (enabled: boolean, variant: string) => { |
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.
issue: is this really needed? I would rather suggest we rely on useExperimentVariant
instead to avoid maintaining two methods that more or less do the same thing. It also works pretty well with our overrides unlike this new method which will also need to be updated to support it.
If there is something broken with useExperimentVariant
, then we have a big problem and we need to fix it 😄 .
Ideally we would to get the variant like this or so:
const experiment = useExperimentVariant(flag)
const variant = experiment.variant
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 added it to avoid implementing the same variant check in multiple files:
const newSaveAndFollowOnArtworkCardExperiment = useExperimentVariant(
"onyx_artwork-card-save-and-follow-cta-redesign"
)
const enableShowOldSaveCTA =
newSaveAndFollowOnArtworkCardExperiment.enabled &&
newSaveAndFollowOnArtworkCardExperiment.payload === "variant-a"
const enableNewSaveCTA =
newSaveAndFollowOnArtworkCardExperiment.enabled &&
newSaveAndFollowOnArtworkCardExperiment.payload === "variant-b"
const enableNewSaveAndFollowCTAs =
newSaveAndFollowOnArtworkCardExperiment.enabled &&
newSaveAndFollowOnArtworkCardExperiment.payload === "variant-c"
It will only be used for this experiment and will be removed when the experiment is finished.
The experiment is taking a long time and I'm afraid, the logic can be messed up eventually is we keep these code duplicated.
I'm open to revert the change if you disagree
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.
since it is currently within the experiments
folder, I am afraid someone would use it accidentally and it would make removing it hard. Do you mind if you maybe rename it to something more specific to your feature and move it closer. Thank you
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.
thanks for addressing my comments. only one last comment left about getExperiment renaming.
This PR resolves ONYX-1293 ONYX-1286
Description
Redesign Save CTA and add Follow CTA to the artwork card and grid item
Implement experiments: variant-a, variant-b and variant-c
Add confirmation toast for the Artist follow action
Hide changes behind a feature flag AREnableNewSaveAndFollowOnArtworkCard
Figma: https://www.figma.com/design/tZ5OjG3dXFmNKFY3zqXlYV/App-Card-and-Rail?node-id=411-12304&node-type=canvas&m=dev
Rail
Grid with 1 column
Grid with 2 columns
Follow Artist toast
Save button on Artwork screen
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.