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.

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

[$250] [Search v2.3] Add Onyx optimistic and failure data #49439

Open
lakchote opened this issue Sep 19, 2024 · 9 comments
Open

[$250] [Search v2.3] Add Onyx optimistic and failure data #49439

lakchote opened this issue Sep 19, 2024 · 9 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lakchote
Copy link
Contributor

lakchote commented Sep 19, 2024

See #48566 (comment)

You should also optimistically display the saved search.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836663116286679327
  • Upwork Job ID: 1836663116286679327
  • Last Price Increase: 2024-09-19
  • Automatic offers:
    • nkdengineer | Contributor | 104037632
Issue OwnerCurrent Issue Owner: @hoangzinh
@lakchote lakchote self-assigned this Sep 19, 2024
@lakchote lakchote added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Sep 19, 2024
@melvin-bot melvin-bot bot changed the title [Search v2.3] Add Onyx optimistic and failure data [$250] [Search v2.3] Add Onyx optimistic and failure data Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021836663116286679327

Copy link

melvin-bot bot commented Sep 19, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External)

@nkdengineer
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add Onyx optimistic and failure data

What is the root cause of that problem?

The Save search button is not greyed out when offline, but instead takes you to the You appear to be offline page which is probably fine for now, but blocking the button tap would be the standard thing to do here I think!

The save search button is not disabled when we offline

Deleting a saved search has no offline feedback (I'd expect the deleted search to show up greyed out)

function deleteSavedSearch(hash: number) {

What changes do you think we should make in order to solve the problem?

  1. The Save search button is not greyed out when offline, but instead takes you to the You appear to be offline page which is probably fine for now, but blocking the button tap would be the standard thing to do here I think!

We can add disabled={isOffline} for the save search button. Or if we want to create optimistic data and failure data for ONYXKEYS.SAVED_SEARCHES with pendingAction as add here with key is queryJSON.hash, name is queryJSON.saveSearchName and query is queryJSON.inputQuery. Then we can also add an error infailureData to display the error

  1. When we delete the saved search here, we can add pendingAction in optimistic data of this saved search as delete and reset it in failureData and we can also add an error if the API fails

function deleteSavedSearch(hash: number) {

Then we can add pendingAction field as item.pendingAction here. We can also add error field to display the error and onPendingActionDismiss to clear the error if we want

const baseMenuItem: SavedSearchMenuItem = {

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 19, 2024
@lakchote
Copy link
Contributor Author

Added @allroundexperts experts as the C+ since he's available now to review, to move this forward.

@allroundexperts
Copy link
Contributor

@nkdengineer's proposal looks good enough to me. I think we want to go with the optimistic data creation in this case. @lakchote can confirm this.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Sep 19, 2024

Current assignee @lakchote is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@lakchote
Copy link
Contributor Author

Let's go with optimistic data creation, yes.

@nkdengineer's proposal LGTM.

Copy link

melvin-bot bot commented Sep 19, 2024

📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 20, 2024
@nkdengineer
Copy link
Contributor

@allroundexperts The PR is here.

@luacmartins luacmartins self-assigned this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Polish
Development

No branches or pull requests

5 participants