Skip to content

Advanced date filter view#45756

Merged
luacmartins merged 16 commits intoExpensify:mainfrom
software-mansion-labs:Guccio163/45026-search-advanced-filters
Jul 24, 2024
Merged

Advanced date filter view#45756
luacmartins merged 16 commits intoExpensify:mainfrom
software-mansion-labs:Guccio163/45026-search-advanced-filters

Conversation

@Guccio163
Copy link
Copy Markdown
Contributor

@Guccio163 Guccio163 commented Jul 19, 2024

Details

This PR adds an Advanced filter "Date" and is an example of underlying logic that will be used for the rest of the filters.

Fixed Issues

$ #46033
PROPOSAL:

Tests

  1. Open search/date page
  2. Select one of or both dates and save
  3. Go back/reload the app
  4. Selected dates should remain set and visible appropriately in DatePickers
  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
search_date_android.mov
Android: mWeb Chrome
Screen.Recording.2024-07-22.at.12.13.14.mov
iOS: Native
Screen.Recording.2024-07-22.at.09.02.20.mov
iOS: mWeb Safari
search_date_iosWeb.mov
MacOS: Chrome / Safari
Screen.Recording.2024-07-22.at.08.44.31.mov
MacOS: Desktop
Screen.Recording.2024-07-22.at.12.50.41.mov

@Guccio163

This comment was marked as duplicate.

@Guccio163

This comment was marked as duplicate.

Copy link
Copy Markdown
Contributor

@Kicu Kicu left a comment

Choose a reason for hiding this comment

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

Good job, the UI code looks good and mostly simple 👍
I left you several comments, most of them quite simple, some renaming and some pointing out of better patterns.

Please take a look

export {search, createTransactionThread, deleteMoneyRequestOnSearch, holdMoneyRequestOnSearch, unholdMoneyRequestOnSearch, exportSearchItemsToCSV};

// Temporary function, ultimately general function to update filters object with any filter, bow only date for test purposes
function mergeFilters(updatedForm: FormOnyxValues<typeof ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM>) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be moved to SearchUtils.ts which is the file to keep helper functions and functions that help with managing data.
actions are strictly for api calls

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@blazejkustra what do You think about that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think @Kicu is right here, let's move to utils file 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks for the help 🙌

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm moving it to SearchUtils.ts, but it requires me to add // eslint-disable-next-line rulesdir/prefer-actions-set-data which I didn't have to add in actions/Search.ts. From looking into other Onyx.merge usages it is the only way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Additionally when writing it in actions/Search.ts I was following convention seen f.ex. in IOURequestStepDate.tsx in which they get function merging in Onyx from actions/IOS.ts : IOU.setDraftSplitTransaction(transaction?.transactionID ?? '-1', {created: newCreated});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After consulting with @blazejkustra I'm moving mergeFilters back to actions/Search

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should move this to src/libs/actions/Search.ts. All Onyx actions should be declared in libs/actions files.


const [searchAdvancedFiltersForm] = useOnyx(ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM);

const isDateAfterInitialized = !!(searchAdvancedFiltersForm && searchAdvancedFiltersForm[INPUT_IDS.DATE.AFTER]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you have to do a lot of work here to make sure searchAdvancedFiltersForm is non null.

This code will be simpler if you do a default value when destructuring array in line 26:

const [foo = {}] = useOnyx(...

then these 2 checks will be much simpler

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I extracted default value into separate const and added it just like you wrote, everything works just fine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left a comment here, but given that these filters are all optional, I don't think it makes sense to add a default value. We should just leave them empty and add only what the user selects

@Guccio163
Copy link
Copy Markdown
Contributor Author

Guccio163 commented Jul 19, 2024

Right now I have 4 questions regarding filters:

  1. How do we want to separate choosing "before" and "after" date picking? We need to allow user to choose only one at a time and right now DatePicker doesn't support unchecking the date.
  2. How are we going to manage filters' forms persistency? We want them to stay set upon page refresh, but not forever :)
  3. I propose to refactor "before X after Y" to "from X to Y". In my opinion it is more intuitive that way, though I understand the decision because of overlapping with "from" and "to" in Members section.
  4. [UI Designer] how do we want to display the calendars, one under another, pop-up or maybe sliding in from under input onPress?

@Kicu
Copy link
Copy Markdown
Contributor

Kicu commented Jul 22, 2024

How are we going to manage filters' forms persistency? We want them to stay set upon page refresh, but not forever :)

I thought that our plan is that:

  • for refreshing of page onyx is enough to keep the current filters state
  • however if we enter the advanced filters page from the results page, then we will overwrite the values in onyx with the values that came from current query in URL; that should work, shouldn't it?

the rest of questions go to @luacmartins

@Guccio163
Copy link
Copy Markdown
Contributor Author

What I meant is a matter of how we want to handle situation in which user starts to set filters and then changes his mind/ closes page and return after f.ex. 2 days? Right now filters would still be set in Onyx.

@Guccio163 Guccio163 marked this pull request as ready for review July 22, 2024 11:28
@Guccio163 Guccio163 requested a review from a team as a code owner July 22, 2024 11:28
@melvin-bot melvin-bot bot removed the request for review from a team July 22, 2024 11:28
@melvin-bot
Copy link
Copy Markdown

melvin-bot bot commented Jul 22, 2024

@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@melvin-bot melvin-bot bot requested a review from rayane-d July 22, 2024 11:28
@luacmartins luacmartins self-requested a review July 22, 2024 17:20
@rayane-d
Copy link
Copy Markdown
Contributor

rayane-d commented Jul 22, 2024

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native
Screen.Recording.2024-07-24.at.1.10.45.PM.mov
Android: mWeb Chrome
Screen.Recording.2024-07-24.at.12.59.48.PM.mov
iOS: Native
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-24.at.12.55.50.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-07-24.at.12.54.37.PM.mov
MacOS: Desktop
Screen.Recording.2024-07-24.at.1.53.58.PM.mov

Copy link
Copy Markdown
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Welcome to the team @Guccio163! Overall the PR looks good, I left a few comments in code. Additionally, there are a couple more things I'd like to request:

  1. Can we add the View results button to the AdvancedSearchFilters page? That way we can test that selecting a value and clicking it will correctly update the URL with the new query string and thus the search results.
  2. Selecting dates and saving doesn't update the MenuItemWithTopDescription with the selected values
Screen.Recording.2024-07-22.at.2.45.55.PM.mov
  1. Currently the UI looks like this:
    Screenshot 2024-07-22 at 2 42 00 PM

However, the doc shows it just as the text input (no calendar). @Expensify/design could you confirm which version we should have for the Search date filter?

Screenshot 2024-07-22 at 2 34 13 PM

export {search, createTransactionThread, deleteMoneyRequestOnSearch, holdMoneyRequestOnSearch, unholdMoneyRequestOnSearch, exportSearchItemsToCSV};

// Temporary function, ultimately general function to update filters object with any filter, bow only date for test purposes
function mergeFilters(updatedForm: FormOnyxValues<typeof ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM>) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should move this to src/libs/actions/Search.ts. All Onyx actions should be declared in libs/actions files.

Comment on lines +28 to +29
const defaultDateAfter = searchAdvancedFiltersForm[INPUT_IDS.DATE_AFTER];
const defaultDateBefore = searchAdvancedFiltersForm[INPUT_IDS.DATE_BEFORE];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should add defaults here. All of these values are optional so the default should be no date selected

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we want default date to not be set, default values should be changed from current date to undefined, but I need to create them so that Typescript is assured that searchAdvancedFiltersForm is not undefined.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should define the ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM values Onyx type in OnyxFormValuesMapping to ensure that searchAdvancedFiltersForm is correctly typed, and remove DEFAULT_FILTERS_FORM_VALUE

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I apply these changes, TS still isn't sure the whole object under Onyx key isn't undefined:
351312560-6c214aad-7a1f-45c3-af2b-7806442c04df
Alternatively, I can assign empty object with a cast, that works as well:
const [searchAdvancedFiltersForm = {} as SearchAdvancedFiltersForm] = useOnyx(ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM)
defaultDateAfter/Before are both extracted just for clarity

Copy link
Copy Markdown
Contributor

@rayane-d rayane-d Jul 24, 2024

Choose a reason for hiding this comment

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

Sorry, I'm on mobile right now so I can't check. But, All Onyx entries can be undefined, and you can use Typescript optional chaining to access their properties with a fallback if you don't want the value to be undefined. You can also search for useOnyx usages in the codebase to see examples. I can give you code suggestions when I get back.

Copy link
Copy Markdown
Contributor Author

@Guccio163 Guccio163 Jul 24, 2024

Choose a reason for hiding this comment

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

Good point, optional chaining went through my mind, but I thought it would be less clean, I'll change it right now

@dannymcclain
Copy link
Copy Markdown
Contributor

Ahh, ok interesting. So (speaking for myself) I didn't realize that the calendar was always present with the date inputs. I thought it only showed up when the input was focused.

Since that's clearly not the case, it seems to me like our options are to:

  • leave it as it is in the screenshots (showing the calendars all the time)
  • adjust both the Before and After inputs to be push inputs, that reveal the actual input (and therefore calendar) once you tap into them

Does that sound correct to you? Super duper interested in the rest of the design team's thoughts. I was honestly kinda surprised to realize how those date inputs actually work!

@luacmartins
Copy link
Copy Markdown
Contributor

How do we want to separate choosing "before" and "after" date picking? We need to allow user to choose only one at a time and right now DatePicker doesn't support unchecking the date.

Haha I made several comments about this, but we need the date to be empty since they are optional.

How are we going to manage filters' forms persistency? We want them to stay set upon page refresh, but not forever :)

I think we should clear the Onyx keys once we close the advanced filter page. IMO every time the user opens the page, we should load the filters based on the user's current search and disregard any previous selected values.

I propose to refactor "before X after Y" to "from X to Y". In my opinion it is more intuitive that way, though I understand the decision because of overlapping with "from" and "to" in Members section.

I think before and after will be a UI only thing since the query syntax actually uses date>2024-01-01 date<2024-07-01, so I think keeping it before and after makes it clearer.

[UI Designer] how do we want to display the calendars, one under another, pop-up or maybe sliding in from under input onPress?

I pinged the design team above. The design doc has just the text input, so I assume pop-up, but I'll let them confirm.

What I meant is a matter of how we want to handle situation in which user starts to set filters and then changes his mind/ closes page and return after f.ex. 2 days? Right now filters would still be set in Onyx.

Answered above. I think we should clear the data when the user navigates away from the page.

@luacmartins
Copy link
Copy Markdown
Contributor

leave it as it is in the screenshots (showing the calendars all the time)
adjust both the Before and After inputs to be push inputs, that reveal the actual input (and therefore calendar) once you tap into them

I'd add a third option which is to modify the date input to allow for a pop up when it's focused. Otherwise, yea these are the options we have.

@dannymcclain
Copy link
Copy Markdown
Contributor

I'd add a third option which is to modify the date input to allow for a pop up when it's focused.

Personally this would probably get my vote, but I really don't know what kind of effort that would require or what any potential gotchas we might encounter would be, so I was hesitant to suggest it haha. @shawnborton @dubielzyk-expensify what do you guys think?

@Kicu
Copy link
Copy Markdown
Contributor

Kicu commented Jul 23, 2024

Sorry, but that is not completely true. What I'm trying to say for the last 3 comments is that it's the input UI "type" itself adding this extra step. This whole UI control consists of 2 screens.
It's not that I used it here in a bad way, it has been created like this for the beginning.

Anyways: I don't want to hijack this PR, so I will code around it, but wanted to point this out for other designers and devs to be aware.
I will make it so that there is only 1 step and no extra clicks.

@shawnborton
Copy link
Copy Markdown
Contributor

Hmm I am really not following what you are saying. We have plenty of examples of screens throughout the app where list selection inputs only use two screens, I don't know why this particular flow was using 3 screens. I can't think of any other place in the app that has it implemented as 3 screens either.

@rayane-d
Copy link
Copy Markdown
Contributor

rayane-d commented Jul 23, 2024

@Kicu - I think we can use ValuePicker component directly in the AdvancedFiltersPage page component without creating AdvancedFiltersTypePage, and use onInputChange to merge the filter value

@Kicu
Copy link
Copy Markdown
Contributor

Kicu commented Jul 23, 2024

@rayane-djouah it's a good suggestion but I'm afraid its not optimal.
if I do it like that then first of all the type page with the list for choosing wont be it's own page with a route, and second of all some of filter inputs would be defined directly in AdvancedFiltersPage and some on their own dedicated pages (like Date) which I think is a bad idea.

@shawnborton yes I understand that you are not following because I started to talk directly about the React Components available in the code, maybe I shouldn't have. What I said is completely true in regards to code, not to the UI flows.

But I know what is the expectation and I'm able to do it, there will be a PR soon

Comment on lines +28 to +29
const defaultDateAfter = searchAdvancedFiltersForm[INPUT_IDS.DATE_AFTER];
const defaultDateBefore = searchAdvancedFiltersForm[INPUT_IDS.DATE_BEFORE];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should define the ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM values Onyx type in OnyxFormValuesMapping to ensure that searchAdvancedFiltersForm is correctly typed, and remove DEFAULT_FILTERS_FORM_VALUE

@dannymcclain
Copy link
Copy Markdown
Contributor

Ok so it sounds like we're down to try the calendar as a popover on focus.

In various flows, we have some push-to-page fields where the only thing we ask is for a date (like birthdate or business incorporation date). In those cases, I think it's actually quite nice that the calendar is displayed by default so it saves you an extra click.

@shawnborton I definitely see your point here, but my counter-argument is that we autofocus these kinds of inputs when you navigate to a page. So basically you would still see the calendar by default because we're immediately focusing the input when you land on the screen.

In regards to the type selector, I agree with Shawn that that just looks like it hasn't been implemented consistently. For example, here is our preferences screen. Each push input takes you directly to the list selection. I believe this is what Shawn was talking about above. Sounds like everyone is already on top of it, just wanted to toss this example in for extra clarity.

CleanShot.2024-07-23.at.08.30.04.mp4

@shawnborton
Copy link
Copy Markdown
Contributor

I definitely see your point here, but my counter-argument is that we autofocus these kinds of inputs when you navigate to a page. So basically you would still see the calendar by default because we're immediately focusing the input when you land on the screen.

Ah I guess that's a fair point. I guess what I'm mostly saying is that I don't think we need to change the existing behavior for this in the places where it's already working well.

I believe this is what Shawn was talking about above.

Indeed it is!

@dannymcclain
Copy link
Copy Markdown
Contributor

Ah I guess that's a fair point. I guess what I'm mostly saying is that I don't think we need to change the existing behavior for this in the places where it's already working well.

Works for me if everyone is cool with that approach!

@luacmartins
Copy link
Copy Markdown
Contributor

@luacmartins I shortened the point I wanted to make there; Even if we allow user to set only one date at the start (both inputs are empty as default), what in a situation in which user selects both dates, confirms (but doesn't click "Show results", he's just redirected to all filters view) and then changes his mind and wants to remove before or after date? Right now he'd have to discard the changes made in all filters, so I think we still need the option to un-check the date.

Makes sense. I think the behavior is:

  1. Not have any default date set
  2. Allow users to select/unselect dates

@rayane-djouah I think we cannot do that, we'd disable users from choosing only the 'DATE_BEFORE' filter.

Correct, we need to display both since they are independent filters.

@shawnborton I see that we all agree that pop-up on focus is a great idea; Should I add that in this very PR, or maybe this should be split into another issue? Seems like a bit of work :)

We can do this in a follow up, not to distract from the original issue.

@rayane-djouah it's a good suggestion but I'm afraid its not optimal.
if I do it like that then first of all the type page with the list for choosing wont be it's own page with a route, and second of all some of filter inputs would be defined directly in AdvancedFiltersPage and some on their own dedicated pages (like Date) which I think is a bad idea.

@Kicu I think this is what we do in other forms. The only reason for the date filter (and some others) to have their own route/page is because they are a bit special, e.g. the date filter has both before and after dates in a single page so we need to create a separate view for that. I'm not sure that that's a requirement for simple pickers like the type/status

@luacmartins
Copy link
Copy Markdown
Contributor

@Guccio163 let me know once this PR is ready for review again

@Guccio163
Copy link
Copy Markdown
Contributor Author

I think that it'll be completed right after we finish with the defaultDates with @rayane-djouah, the only other (and biggest) issue which is customising DatePicker will be converted into another ticket.

@Guccio163
Copy link
Copy Markdown
Contributor Author

@rayane-djouah just letting you know that I applied optional chaining and deleted default values, because GitHub hid it in Files changed due to comment being outdated; Please let me know if it's okay :)

@Guccio163
Copy link
Copy Markdown
Contributor Author

@luacmartins I see that @rayane-djouah has completed the Reviewer Checklist, I suppose this is sign that it's ready for the review again 🤞

Copy link
Copy Markdown
Contributor

@rayane-d rayane-d left a comment

Choose a reason for hiding this comment

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

NAB comment

InputComponent={DatePicker}
inputID={INPUT_IDS.DATE_BEFORE}
label={translate('search.filters.date.before')}
defaultValue={searchAdvancedFiltersForm?.[INPUT_IDS.DATE_BEFORE]}
Copy link
Copy Markdown
Contributor

@rayane-d rayane-d Jul 24, 2024

Choose a reason for hiding this comment

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

same here for searchAdvancedFiltersForm?.[INPUT_IDS.DATE_BEFORE]

Copy link
Copy Markdown
Contributor

@rayane-d rayane-d left a comment

Choose a reason for hiding this comment

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

Thank you!
Changes look good to me and test well. The reviewer checklist is also complete.
🎀👀🎀 C+ reviewed

@melvin-bot melvin-bot bot requested a review from luacmartins July 24, 2024 13:24
Comment on lines +24 to +25
const dateAfter = searchAdvancedFiltersForm?.[INPUT_IDS.DATE_AFTER];
const dateBefore = searchAdvancedFiltersForm?.[INPUT_IDS.DATE_BEFORE];
Copy link
Copy Markdown
Contributor

@rayane-d rayane-d Jul 24, 2024

Choose a reason for hiding this comment

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

@Guccio163 - Another NAB comment 😄: We can define INPUT_IDS.DATE_AFTER and INPUT_IDS.DATE_BEFORE as two constants outside the component (line 18) with the names DATE_AFTER_KEY and DATE_BEFORE_KEY just to make these two lines super clear

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Kicu maybe we can address this in your PR?

Comment on lines +24 to +25
const dateAfter = searchAdvancedFiltersForm?.[INPUT_IDS.DATE_AFTER];
const dateBefore = searchAdvancedFiltersForm?.[INPUT_IDS.DATE_BEFORE];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Kicu maybe we can address this in your PR?

@luacmartins luacmartins merged commit 0cde0b5 into Expensify:main Jul 24, 2024
@OSBotify
Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.12-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Guccio163 Guccio163 deleted the Guccio163/45026-search-advanced-filters branch July 25, 2024 18:42
@OSBotify
Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/francoisl in version: 9.0.12-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.13-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants