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

[HOLD for payment 2024-10-11] [HOLD for payment 2024-10-10] [$250] [Search v2.2] Reload the page and highlight the newly added expense when adding a new expense from the search page #48716

Closed
trjExpensify opened this issue Sep 6, 2024 · 43 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Design External Added to denote the issue can be worked on by a contributor

Comments

@trjExpensify
Copy link
Contributor

trjExpensify commented Sep 6, 2024

Follow-up to here.

Two items to add:

  1. When the new expense is added to the search page, trigger a reload of the page so it can appear in the table.

I think we might need to just trigger a reload of the page given the existing filters may or may not show the new transation.

  1. Highlight the newly added expense row in the table when it appears

For this, we were thinking it could borrow from the workspace settings when you enable a new feature in More features, and the row gets highlighted in the LHN briefly.

Once we've done that, we'll add this regression test for Applause:

  1. Switch to the search page tab
  2. Click the global create button > submit expense > submit an expense
  3. Verify you remain on the search page
  4. Verify the search page reloads and the expense appears in the table
  5. Verify the newly added expense row is briefly highlighted to draw attention to it

CC: @Expensify/design @luacmartins @adamgrzybowski @ikevin127

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838256578746413929
  • Upwork Job ID: 1838256578746413929
  • Last Price Increase: 2024-09-23
  • Automatic offers:
    • ikevin127 | Reviewer | 104270100
    • ishpaul777 | Contributor | 104270102
Issue OwnerCurrent Issue Owner: @trjExpensify
@trjExpensify trjExpensify added Daily KSv2 Planning Changes still in the thought process Bug Something is broken. Auto assigns a BugZero manager. labels Sep 6, 2024
@trjExpensify trjExpensify self-assigned this Sep 6, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Sep 6, 2024

Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.

@trjExpensify
Copy link
Contributor Author

Can we get a mock of the row highlighted to include here, please? Thanks!

@ikevin127
Copy link
Contributor

♻️ Will look into this tomorrow, the plan being to come up with a proposal and some videos for the design team to review 👍

@melvin-bot melvin-bot bot added the Overdue label Sep 9, 2024
@dubielzyk-expensify
Copy link
Contributor

Animation 1: Just color fade

CleanShot.2024-09-09.at.20.06.20.mp4

Animation 2: Slide and color fade

CleanShot.2024-09-09.at.20.06.36.mp4

Animation 3: Color fade and stay

CleanShot.2024-09-09.at.20.10.45.mp4

No animation: Use highlight state color

We do use a highlight color on chats but I'm personally not a huge fan of this due to how muddy it looks. If we apply it here I don't think it looks great:
image

cc @Expensify/design team for thoughts

@melvin-bot melvin-bot bot removed the Overdue label Sep 9, 2024
@shawnborton
Copy link
Contributor

Slide and color fade is by far my favorite! If that's not possible, I think I lean towards just the color fade. Fade and stay feels a bit weird in that it's almost like the expense row is selected. And I agree about the muddy color 🙈

@dannymcclain
Copy link
Contributor

Slide and color fade is by far my favorite! If that's not possible, I think I lean towards just the color fade.

100% agree. Slide and color fade is :chefkiss: but if that's not feasible, color fade is still great.

@dubielzyk-expensify what color are you using for the "highlight"? I almost wonder if we should make it a bit more obvious since we just want a quick little fade?

PS. It's a little sad to me that we have a highlight color variable but we don't want to use it because it doesn't look great... I feel like it might be worth revisiting and fixing that color because this situation is essentially exactly what that variable is for: a highlight.

@shawnborton
Copy link
Contributor

Yeah, that's a fair sentiment. I think the reality is that it looks bad on dark mode, but it looks pretty normal for dark mode themes. In light mode, I think it feels better and uses an actual brand color right?

@dannymcclain
Copy link
Contributor

I think the reality is that it looks bad on dark mode, but it looks pretty normal for dark mode themes.

Yeah I do agree with that.

In light mode, I think it feels better and uses an actual brand color right?

Yes, in light mode it's just set to yellow-100 (and it looks great haha).

@dubielzyk-expensify
Copy link
Contributor

Yeah agree that in light mode it looks great. Maybe we just revisit the dark mode highlight cause I think you're right Danny that we should use this semantic color since it has a purpose.

Slide looks good but I made a version without it cause I worried about performance if the list is huge. I'm happy with either. I think the stay is my least preferred. We can do the normal fade and just delay the fade if we want anyways. Also with the stay version we'd have to address the checkbox and receipt color stuff which I really don't wanna do haha.

@dubielzyk-expensify what color are you using for the "highlight"? I almost wonder if we should make it a bit more obvious since we just want a quick little fade?

I used row-highlight in that example cause I didn't want it to cross over into select land (button), but I agree that it's a tad subtle.

Here's updated slide-in with highlight color for dark and light mode:

CleanShot.2024-09-10.at.11.05.59.mp4
CleanShot.2024-09-10.at.11.05.14.mp4

@ikevin127
Copy link
Contributor

Thanks for confirming the animation we want here! 🚀

♻️ Made some progress today functionality wise (see video below), the only thing left to do is to integrate the animation which so far it looks like everybody is in favor of the Animation 2: Slide and color fade, please let me know if we want something different - anyhow I will come back tomorrow with the final version including animation present a new video for confirmation 👍

Video
Screen.Recording.2024-09-09.at.19.19.44.mov

@dubielzyk-expensify
Copy link
Contributor

Awesome. Yeah let's wait until @dannymcclain and @shawnborton have given their thoughts 😄

@shawnborton
Copy link
Contributor

I definitely appreciate how the slide fade with highlight color makes things more obvious and is a bit more connected to the color we use for comment highlighting. Maybe we go with that and then follow up and pick a better color for dark mode? I can also get down with just the slide fade using border color too.

@dubielzyk-expensify
Copy link
Contributor

100% down for that. Let me add a follow-up task for myself to explore the dark mode highlight color. I think that's the right call, though keen to hear if @dannymcclain agrees?

@trjExpensify
Copy link
Contributor Author

Assigning @ikevin127, forgot to do that. Also assigning @luacmartins for the internal review portion of the PR when ready.

@dannymcclain
Copy link
Contributor

I totally agree! That sounds like a great plan to me. And I also think that if the slide animation can't work because of performance, just doing the highlight animation will still be great.

@trjExpensify trjExpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 23, 2024
@trjExpensify
Copy link
Contributor Author

Just adding the External label to get a job created for Kevin here.

Copy link

melvin-bot bot commented Sep 30, 2024

@trjExpensify, @luacmartins, @ikevin127, @dubielzyk-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@ikevin127
Copy link
Contributor

ikevin127 commented Sep 30, 2024

♻️ The PR is about to be merged, just resolved latest conflicts and C+ review passed.

Edit: PR was merged.

Copy link

melvin-bot bot commented Oct 2, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 3, 2024
@melvin-bot melvin-bot bot changed the title [$250] [Search v2.2] Reload the page and highlight the newly added expense when adding a new expense from the search page [HOLD for payment 2024-10-10] [$250] [Search v2.2] Reload the page and highlight the newly added expense when adding a new expense from the search page Oct 3, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 3, 2024
Copy link

melvin-bot bot commented Oct 3, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Oct 3, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.43-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-10. 🎊

For reference, here are some details about the assignees on this issue:

  • @ikevin127 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Oct 3, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@ikevin127] The PR that introduced the bug has been identified. Link to the PR:
  • [@ikevin127] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@ikevin127] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@ikevin127] Determine if we should create a regression test for this bug.
  • [@ikevin127] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@ishpaul777
Copy link
Contributor

👋 Please assign me here i reviewed #49192

Copy link

melvin-bot bot commented Oct 4, 2024

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

Offer link
Upwork job

Copy link

melvin-bot bot commented Oct 4, 2024

📣 @ishpaul777 🎉 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 Weekly KSv2 and removed Weekly KSv2 labels Oct 4, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-10-10] [$250] [Search v2.2] Reload the page and highlight the newly added expense when adding a new expense from the search page [HOLD for payment 2024-10-11] [HOLD for payment 2024-10-10] [$250] [Search v2.2] Reload the page and highlight the newly added expense when adding a new expense from the search page Oct 4, 2024
Copy link

melvin-bot bot commented Oct 4, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.44-12 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-11. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Oct 4, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@ikevin127 / @ishpaul777] The PR that introduced the bug has been identified. Link to the PR:
  • [@ikevin127 / @ishpaul777] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@ikevin127 / @ishpaul777] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@ikevin127 / @ishpaul777] Determine if we should create a regression test for this bug.
  • [@ikevin127 / @ishpaul777] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@ikevin127
Copy link
Contributor

Regression Test Proposal

  1. Switch to the Search page tab.
  2. Click the global create button (FAB) > Submit expense > Submit an expense.
  3. Verify you remain on the Search page.
  4. Verify that the newly added expense appears in the table and the row is briefly highlighted to draw attention to it.

Do we agree 👍 or 👎.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 9, 2024
@trjExpensify
Copy link
Contributor Author

Regression test created. Payment summary as follows:

Paid, closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Design External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

7 participants