feat: Inline price control create alert #8835

rajsam003 · 2023-06-08T15:12:34Z

This PR resolves FX-4806

This PR adds Inline price control for Create Alert modal flow

Here is the figma spec

Screen.Recording.2023-06-12.at.15.49.03.mov

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

Inline price control in create/edit alert - Sam

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.

src/app/Scenes/SavedSearchAlert/Components/Form.tsx

src/app/Scenes/SavedSearchAlert/screens/CreateSavedSearchAlertPriceRangeScreen.tsx

src/app/Scenes/SavedSearchAlert/Components/Form.tsx

src/app/Scenes/SavedSearchAlert/screens/CreateSavedSearchAlertPriceRangeScreen.tsx

src/app/Scenes/SavedSearchAlert/Components/Form.tsx

rajsam003 · 2023-06-09T15:10:47Z

src/app/Scenes/SavedSearchAlert/SavedSearchStore.tsx

+    //@ts-ignore
+    state.attributes[payload.key] = payload.value


I find it hard to fix this type error. Any help is appreciated.

src/app/Scenes/SavedSearchAlert/CreateSavedSearchAlert.tsx

src/app/Scenes/SavedSearchAlert/screens/CreateAlertPriceRangeScreen.tsx

anandaroop

Looking nice! Just one question from me about an unimplemented feature (maybe more of a PM/design question).

anandaroop · 2023-06-12T22:17:37Z

src/app/Scenes/SavedSearchAlert/screens/AlertPriceRangeScreen.tsx

+        <Flex m={2} p={2} alignItems="center" backgroundColor="black5">
+          <Text variant="sm-display" color="black60">
+            Your recent price ranges will show here
+          </Text>
+        </Flex>


question: We are not implementing this as part of this PR, correct?

If that's the case I wonder if we can hide this section for now, since it's not really helpful or actionable yet.

Good point. Let me ask this in this channel.

To clarify: the "Recent price ranges" section should be in scope for this ticket so I would expect this component, which includes this empty state text to be ported over here.

Erin's comment around the value of the empty state is a good one. We can make that change within the RecentPriceRanges component so that it applies to any screen rendering it.

nickskalkin · 2023-06-13T12:49:57Z

@rajsam003 a tiny bug I've discovering while testing the feature:

Monosnap.screencast.2023-06-13.14-48-49.mp4

nickskalkin · 2023-06-13T12:52:59Z

@rajsam003 "Clear" button is missing in design, but I think we should add it (we discussed it on the refinement session). The button exists on the existing price filter screen:

gkartalis · 2023-06-14T06:57:51Z

src/app/Scenes/SavedSearchAlert/CreateSavedSearchAlert.tsx

+              // force it to not use react-native-screens, which is broken inside a react-native Modal for some reason
+              detachInactiveScreens={false}


tell me more 👀 we might need to be more proactive about this in other surfaces maybe, how did you find out?

@gkartalis maybe you and @rajsam003 can find some time to pair on this? Looks like this argument, and the same comment, exists already in a few different screens.

good point, yes def @rajsam003 feel free to drop some time next week on my cal

As I said I referred other screens as well ( for eg. here, here. It seemed like its generally the case in lot of places. So I find it safer to use this prop.

We looked into it with @rajsam003, turns out that there is an issue with react native modals as the comment suggests, not sure how we can resolve it and remove it but we confirmed that removing this line it breaks the flow (the set price screen modal cannot open).

so lets leave it as it is (MOPLAT will further investigate when migrating to react-navigation.

gkartalis · 2023-06-14T07:23:04Z

src/app/Scenes/SavedSearchAlert/Components/Form.tsx

+      <Touchable
+        accessibilityLabel="Set price range"
+        accessibilityRole="button"
+        underlayColor="black5"


I think you can leave this out, no longer needed unless you want a custom color (you can see that removing it has the same behavior while pressing it)

Suggested change

underlayColor="black5"

gkartalis · 2023-06-14T07:24:21Z

Looks great generally. I leave the approval though to the @artsy/fx-devs ⚡️ (btw checked the type issue and wasn't able to fix it quickly but might be worth trying out stuff to make it safe to prevent crashes)

src/app/Scenes/SavedSearchAlert/CreateSavedSearchAlert.tsx

src/app/Scenes/SavedSearchAlert/EditSavedSearchAlertContent.tsx

MounirDhahri

LGTM

src/app/Scenes/SavedSearchAlert/screens/AlertPriceRangeScreen.tsx

dblandin

Perhaps too big of a refactor for this particular PR but I'd be interested if we could extract some of the components within the PriceRangeOptionsScreen, particularly this section and share that between the artwork grid and create alert flows.

Happy to pair on this sometime to explore!

dblandin · 2023-06-16T11:26:30Z

src/app/Scenes/SavedSearchAlert/EditSavedSearchAlert.tsx

+                initialParams={params}
+              />
+              <Stack.Screen name="EmailPreferences" component={EmailPreferencesScreen} />
+              <Stack.Screen name="AlertPriceRangeScreen" component={AlertPriceRangeScreen} />


suggestion: Where is name used here? We should probably be consistent with including or omitting the Screen suffix in the string value.

rajsam003 · 2023-06-16T13:19:09Z

Perhaps too big of a refactor for this particular PR but I'd be interested if we could extract some of the components within the PriceRangeOptionsScreen, particularly this section and share that between the artwork grid and create alert flows.

Happy to pair on this sometime to explore!

I like resuing components. In this case, it was too hectic. I felt the current one is easier and understandable. If anything is messed up in one screen it will not effect the other screen. But yes I can give a try again. 👍🏽

dblandin · 2023-06-16T13:22:40Z

I felt the current one is easier and understandable. If anything is messed up in one screen it will not effect the other screen. But yes I can give a try again.

It be a follow-up PR if you like. The other screen has a lot of coupling with artwork filters but I think there might be a useful component to extract/define that wraps all of the input components and takes function props to respond to value changes.

gkartalis

Looking good, also did a quick QA and played around with some cases and nothing seems to be blocking.

Lets 🛳️ it???? 🚀

Non blocking but for the future:

I remember even back when I was in FX we had this bug that I am not sure if we have ticketed somewhere:

as you see on the screenshot the users are able to create a price range with min price > max price. Let's follow up and create a ticket about that

gkartalis · 2023-06-20T10:56:30Z

src/app/Scenes/SavedSearchAlert/CreateSavedSearchAlert.tsx

+              // force it to not use react-native-screens, which is broken inside a react-native Modal for some reason
+              detachInactiveScreens={false}


We looked into it with @rajsam003, turns out that there is an issue with react native modals as the comment suggests, not sure how we can resolve it and remove it but we confirmed that removing this line it breaks the flow (the set price screen modal cannot open).

so lets leave it as it is (MOPLAT will further investigate when migrating to react-navigation.

rajsam003 requested review from MounirDhahri, gkartalis and a team June 8, 2023 15:12

rajsam003 self-assigned this Jun 8, 2023

artsy-peril bot added the Jira Synced label Jun 8, 2023

gkartalis reviewed Jun 8, 2023

View reviewed changes

src/app/Scenes/SavedSearchAlert/Components/Form.tsx Outdated Show resolved Hide resolved

src/app/Scenes/SavedSearchAlert/screens/CreateSavedSearchAlertPriceRangeScreen.tsx Outdated Show resolved Hide resolved

MounirDhahri reviewed Jun 8, 2023

View reviewed changes

rajsam003 commented Jun 9, 2023

View reviewed changes

src/app/Scenes/SavedSearchAlert/CreateSavedSearchAlert.tsx Show resolved Hide resolved

rajsam003 commented Jun 9, 2023

View reviewed changes

src/app/Scenes/SavedSearchAlert/screens/CreateAlertPriceRangeScreen.tsx Outdated Show resolved Hide resolved

rajsam003 added 5 commits June 12, 2023 09:32

inline price control create alert

2e75feb

update

37ec7f0

revert podlock file

9cf5c7e

new screen

dfc9669

with Edit Alert flow

56592d6

rajsam003 force-pushed the rajsam003/FX-4806/inline-price-control-create-alert branch from 2e5c201 to 56592d6 Compare June 12, 2023 13:13

price update

0ddc6cf

rajsam003 requested review from MounirDhahri, gkartalis and anandaroop June 12, 2023 13:50

rajsam003 marked this pull request as ready for review June 12, 2023 13:50

rajsam003 requested a review from damassi June 12, 2023 14:51

anandaroop reviewed Jun 12, 2023

View reviewed changes

remove section

dff9b55

gkartalis reviewed Jun 14, 2023

View reviewed changes

MounirDhahri reviewed Jun 14, 2023

View reviewed changes

src/app/Scenes/SavedSearchAlert/CreateSavedSearchAlert.tsx Show resolved Hide resolved

src/app/Scenes/SavedSearchAlert/EditSavedSearchAlertContent.tsx Show resolved Hide resolved

MounirDhahri previously approved these changes Jun 16, 2023

View reviewed changes

src/app/Scenes/SavedSearchAlert/screens/AlertPriceRangeScreen.tsx Outdated Show resolved Hide resolved

src/app/Scenes/SavedSearchAlert/screens/AlertPriceRangeScreen.tsx Show resolved Hide resolved

dblandin reviewed Jun 16, 2023

View reviewed changes

updated with PR

add643e

rajsam003 dismissed MounirDhahri’s stale review via add643e June 16, 2023 11:37

rajsam003 requested a review from dimatretyak June 16, 2023 14:17

AlertPriceRangeScreen to AlertPriceRange

4a41fef

gkartalis approved these changes Jun 20, 2023

View reviewed changes

rajsam003 merged commit 707a3b4 into main Jun 20, 2023

rajsam003 deleted the rajsam003/FX-4806/inline-price-control-create-alert branch June 20, 2023 11:11

nickskalkin mentioned this pull request Jun 22, 2023

chore: temporarily disable inline price control for create alerts flow #8921

Merged

8 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Inline price control create alert #8835

feat: Inline price control create alert #8835

rajsam003 commented Jun 8, 2023 •

edited

Loading

rajsam003 Jun 9, 2023

anandaroop left a comment

anandaroop Jun 12, 2023

rajsam003 Jun 13, 2023

dblandin Jun 16, 2023

dblandin Jun 16, 2023

nickskalkin commented Jun 13, 2023

nickskalkin commented Jun 13, 2023

gkartalis Jun 14, 2023

dblandin Jun 16, 2023

gkartalis Jun 16, 2023

rajsam003 Jun 16, 2023

gkartalis Jun 20, 2023

gkartalis Jun 14, 2023

gkartalis commented Jun 14, 2023

MounirDhahri left a comment

dblandin left a comment

dblandin Jun 16, 2023

rajsam003 commented Jun 16, 2023 •

edited

Loading

dblandin commented Jun 16, 2023 •

edited

Loading

gkartalis left a comment

gkartalis Jun 20, 2023

		// force it to not use react-native-screens, which is broken inside a react-native Modal for some reason
		detachInactiveScreens={false}

feat: Inline price control create alert #8835

feat: Inline price control create alert #8835

Conversation

rajsam003 commented Jun 8, 2023 • edited Loading

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

anandaroop 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

nickskalkin commented Jun 13, 2023

nickskalkin commented Jun 13, 2023

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

Choose a reason for hiding this comment

gkartalis commented Jun 14, 2023

MounirDhahri left a comment

Choose a reason for hiding this comment

dblandin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsam003 commented Jun 16, 2023 • edited Loading

dblandin commented Jun 16, 2023 • edited Loading

gkartalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajsam003 commented Jun 8, 2023 •

edited

Loading

rajsam003 commented Jun 16, 2023 •

edited

Loading

dblandin commented Jun 16, 2023 •

edited

Loading