fix: fix progressive onboarding is not showing up #11404

dariakoko · 2025-01-16T06:42:45Z

This PR resolves []

Description

This PR fixes progressive onboarding. Some of the steps of the onboarding were not showing up blocking the onboarding flow and preventing future work with it.
Slack thread: https://artsy.slack.com/archives/C02BAQ5K7/p1736527757095689

PR Checklist

I have tested my changes on iOS and Android.
I hid my changes behind a feature flag, or they don't need one.
I have included screenshots or videos, or I have not changed the UI.
I have added tests, or my changes don't require any.
I added an app state migration, or my changes do not require one.
I have documented any follow-up work that this PR will require, or it does not require any.
I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

I would like at least one of the reviewers to run this PR on the simulator or device.

Changelog updates

Cross-platform user-facing changes

fix progressive onboarding is not showing up

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.

dariakoko · 2025-01-16T06:44:07Z

src/app/Components/ArtworkGrids/InfiniteScrollArtworksGrid.tsx

@@ -339,7 +336,6 @@ const InfiniteScrollArtworksGrid: React.FC<Props & PrivateProps> = ({
        const displayToolTip =
          enableMyCollectionSellOnboarding &&
          isMyCollection &&
-          !isDismissed(PROGRESSIVE_ONBOARDING_MY_COLLECTION_SELL_THIS_WORK).status &&


moving this into the MyCollectionArtworkGridItem.tsx to have all logic in one place

dariakoko · 2025-01-16T06:45:05Z

src/app/Components/ProgressiveOnboarding/ProgressiveOnboardingAlertFinish.tsx

+  /**
+   * using ToolTip instead of Popover here because we cannot display a Popover inside
+   * of a modal (which is where this component is used)
+   *  */


adding comments for clarity

dariakoko · 2025-01-16T06:48:56Z

src/app/Components/ProgressiveOnboarding/ProgressiveOnboardingSaveAlert.tsx

    // we only enable the alerts flow if the save artwork is completed
-    isDismissed("save-highlight").status &&
+    isDismissed(PROGRESSIVE_ONBOARDING_SAVE_ARTWORK) &&


replacing the strings here because save-highlight is no longer in use and is removed in this PR

dariakoko · 2025-01-16T06:50:13Z

src/app/Scenes/ArtworkLists/ArtworkLists.tsx

-export const ArtworkListsQR = () => {
-  useDismissSavedHighlight()
-
-  return (
-    <Suspense fallback={<ArtworkListsPlaceHolder />}>
-      <ArtworkLists />
-    </Suspense>
-  )
-}
-


not used anywhere - removing

dariakoko · 2025-01-16T07:06:58Z

I am removing the isActive variable because for me it was always false because isActivePopover is always undefined. If you prefer to keep it, would love to hear your thoughts on how to make it work better 🙏

olerichter00

Thanks for all the code cleaning! 🧹

Do you know the purpose of isActive? Understanding would be great to avoid issues when we remove it.

olerichter00 · 2025-01-16T09:09:54Z

src/app/Components/ProgressiveOnboarding/ProgressiveOnboardingSaveAlert.tests.tsx

@@ -54,7 +54,7 @@ describe("ProgressiveOnboardingSaveAlert", () => {
    __globalStoreTestUtils__?.injectState({
      progressiveOnboarding: {
        sessionState: { isReady: true },
-        dismissed: [{ key: "save-highlight", timestamp: Date.now() }],
+        dismissed: [{ key: "offer-settings", timestamp: Date.now() }],


save-highlight is no longer used, the previously dismissed popover has to be offer-settings

or are you referring to the string used instead of the const?

olerichter00 · 2025-01-16T09:10:14Z

src/app/Components/ProgressiveOnboarding/ProgressiveOnboardingSaveAlert.tsx


  const handleDismiss = () => {
    setIsReady(false)
-    dismiss("alert-create")
+    dismiss(PROGRESSIVE_ONBOARDING_ALERT_CREATE)


dariakoko · 2025-01-16T10:23:52Z

Sure @olerichter00, isActive is coming from useSetActivePopover.ts and is used to tell whether there is an active popover on the app. It was done to make sure we don't show 1+ popovers on the screen at the same time. Somehow for me isActive is always false because activePopover variable from useSetActivePopover is always undefined. I couldn't figure out why and I suspect that it could be a bug that might have been introduced due to the laters additions to the progressive onboarding. I am not completely sure whether it's fine to remove it of if there is another way, would be open to hear feedbacks

ArtsyOpenSource · 2025-01-16T12:02:14Z

This PR contains the following changes:

Cross-platform user-facing changes (fix progressive onboarding is not showing up - dariakoko)

Generated by 🚫 dangerJS against ea6cbac

olerichter00 · 2025-01-16T13:27:03Z

I am not completely sure whether it's fine to remove it of if there is another way, would be open to hear feedbacks

Thanks for the explanation 🙏

If isActive prevents the display of two tooltips at the same time, we may need to fix it, find an alternative solution, or ensure that this cannot happen without an additional mechanism to prevent it.

I would be okay with removing the check for the upcoming release to display tooltips again. But then we should definitely look into it more.

araujobarret

looks legit 👍

araujobarret · 2025-01-16T13:46:03Z

src/app/Components/ProgressiveOnboarding/ProgressiveOnboardingOfferSettings.tsx

@@ -19,16 +23,16 @@ export const ProgressiveOnboardingOfferSettings: React.FC = ({ children }) => {
  const isDisplayable =
    isArtworkListOfferabilityEnabled &&
    isReady &&
-    !isDismissed("offer-settings").status &&
-    !!isDismissed("signal-interest").status &&
+    !isDismissed(PROGRESSIVE_ONBOARDING_OFFER_SETTINGS).status &&


not sure about these changes since the template literal value is also auto-completed by IDEs, but I assume maybe the constants are clearer in some way?

Hm, yeah, you're right, I guess it doesn't matter then. Will revert this change, a string is way more readable

dariakoko · 2025-01-16T13:49:01Z

@olerichter00 agree. I think for now the alternative solution would be making sure that popovers that can be displayed on the same screen are dismissed. We can do it manually. We have only one popover that can conflict with other - save-artwork. I added a check to two screens that can display this popover to make sure we do not proceed with the onboarding before it is dismissed.

olerichter00 · 2025-01-16T13:59:28Z

@dariakoko Ok, for now let's test manually to make sure that the case of more than one tooltip at the same time cannot happen. But I think we need to have a general mechanism in the onboarding to prevent it from happening in the future when we add more onboarding tooltips.

dariakoko added 4 commits January 14, 2025 18:25

chore: use imported values

ecb8f65

chore: cleanup InfiniteScrollArtworksGrid component

d8695f4

chore: remove isActive check

50572fe

chore: add save-artwork check

6280a9e

dariakoko commented Jan 16, 2025

View reviewed changes

dariakoko requested review from olerichter00, araujobarret and gkartalis January 16, 2025 07:07

olerichter00 reviewed Jan 16, 2025

View reviewed changes

dariakoko self-assigned this Jan 16, 2025

dariakoko marked this pull request as ready for review January 16, 2025 11:59

araujobarret previously approved these changes Jan 16, 2025

View reviewed changes

chore: revert imports

1cfa721

artsyit dismissed araujobarret’s stale review via 1cfa721 January 16, 2025 14:48

Merge branch 'main' into chore/cleanup-progressive-onboarding-logic

ea6cbac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix progressive onboarding is not showing up #11404

fix: fix progressive onboarding is not showing up #11404

dariakoko commented Jan 16, 2025 •

edited

Loading

dariakoko Jan 16, 2025

dariakoko Jan 16, 2025

dariakoko Jan 16, 2025

dariakoko Jan 16, 2025

dariakoko commented Jan 16, 2025

olerichter00 left a comment

olerichter00 Jan 16, 2025

dariakoko Jan 16, 2025

dariakoko Jan 16, 2025

olerichter00 Jan 16, 2025

dariakoko commented Jan 16, 2025

ArtsyOpenSource commented Jan 16, 2025 •

edited

Loading

olerichter00 commented Jan 16, 2025

araujobarret left a comment

araujobarret Jan 16, 2025

dariakoko Jan 16, 2025

dariakoko commented Jan 16, 2025

olerichter00 commented Jan 16, 2025

fix: fix progressive onboarding is not showing up #11404

Are you sure you want to change the base?

fix: fix progressive onboarding is not showing up #11404

Conversation

dariakoko commented Jan 16, 2025 • edited Loading

Description

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariakoko commented Jan 16, 2025

olerichter00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariakoko commented Jan 16, 2025

ArtsyOpenSource commented Jan 16, 2025 • edited Loading

This PR contains the following changes:

olerichter00 commented Jan 16, 2025

araujobarret left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariakoko commented Jan 16, 2025

olerichter00 commented Jan 16, 2025

dariakoko commented Jan 16, 2025 •

edited

Loading

ArtsyOpenSource commented Jan 16, 2025 •

edited

Loading