Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add "Add filters" to create alert screen #9330

Merged
merged 6 commits into from
Sep 28, 2023
Merged

feat: add "Add filters" to create alert screen #9330

merged 6 commits into from
Sep 28, 2023

Conversation

MounirDhahri
Copy link
Member

@MounirDhahri MounirDhahri commented Sep 26, 2023 •
edited
Loading

This PR resolves ONYX-368 ONYX-371

Description

This PR makes the following changes:

  • Add feature flag
  • Add "Add filters" button to Create an alert screen.
  • Update the UI to match the new designs (I created a follow-up ticket)
  • Navigate to "Create Alert" instead of the "Filter & Sort" modal on the Create Alert press

Group 1 (7)

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

Changelog updates

Cross-platform user-facing changes

  • Add "Add Filters" to Create Alert modal - mounir

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.

@MounirDhahri MounirDhahri self-assigned this Sep 26, 2023
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Sep 26, 2023 •
edited
Loading

This PR contains the following changes:

  • Cross-platform user-facing changes (Add "Add Filters" to Create Alert modal - mounir)

Generated by 🚫 dangerJS against 78f680f

@MounirDhahri MounirDhahri force-pushed the feat/ONYX-368/add-filters-feature-flag branch from c134128 to 1e4ffec Compare September 26, 2023 14:35
@MounirDhahri MounirDhahri marked this pull request as ready for review September 28, 2023 07:45
@MounirDhahri MounirDhahri changed the title feat: add "Add filters" to create alert screen [WIP] feat: add "Add filters" to create alert screen Sep 28, 2023
nickskalkin
nickskalkin previously approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@nickskalkin nickskalkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

dblandin
dblandin reviewed Sep 28, 2023
View reviewed changes
src/app/Scenes/SavedSearchAlert/Components/Form.tsx Outdated
</Box>
{!!priceControlEnabled && (
<>
<Join separator={enableAlertsFilters ? <Separator my={1} /> : <></>}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion / blocking: Are the separator lines intentional? I'm not seeing those in the design spec:
Screenshot 2023-09-28 at 14 29 02

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤯

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took me a while to get them to show up properly 😄 i am sure there were separators but can't find them anymore 😕

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove them because I can't find them anymore

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at mobile web 😢

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done ✅

Copy link
Member Author

@MounirDhahri MounirDhahri Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simulator_screenshot_ED73C7C1-8BDB-4A13-922E-875FCE6B94A1

@MounirDhahri MounirDhahri force-pushed the feat/ONYX-368/add-filters-feature-flag branch from 1e4ffec to 78f680f Compare September 28, 2023 13:48
@MounirDhahri MounirDhahri added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Sep 28, 2023
@artsy-peril artsy-peril bot merged commit f52ec07 into main Sep 28, 2023
9 checks passed
@artsy-peril artsy-peril bot deleted the feat/ONYX-368/add-filters-feature-flag branch September 28, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblandin dblandin dblandin left review comments

@nickskalkin nickskalkin nickskalkin left review comments

@tam-kis tam-kis Awaiting requested review from tam-kis

Assignees

@MounirDhahri MounirDhahri

Labels
Jira Synced Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MounirDhahri @ArtsyOpenSource @dblandin @nickskalkin