-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Comments
Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new. |
Triggered auto assignment to @dubielzyk-expensify ( |
Can we get a mock of the row highlighted to include here, please? Thanks! |
♻️ Will look into this tomorrow, the plan being to come up with a proposal and some videos for the design team to review 👍 |
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 🙈 |
100% agree. Slide and color fade is @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 |
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? |
Yeah I do agree with that.
Yes, in light mode it's just set to |
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
I used Here's updated CleanShot.2024-09-10.at.11.05.59.mp4CleanShot.2024-09-10.at.11.05.14.mp4 |
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 VideoScreen.Recording.2024-09-09.at.19.19.44.mov |
Awesome. Yeah let's wait until @dannymcclain and @shawnborton have given their thoughts 😄 |
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. |
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? |
Assigning @ikevin127, forgot to do that. Also assigning @luacmartins for the internal review portion of the PR when ready. |
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 |
Just adding the |
@trjExpensify, @luacmartins, @ikevin127, @dubielzyk-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick! |
♻️ The PR is about to be merged, just resolved latest conflicts and C+ review passed. Edit: PR was merged. |
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. |
|
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:
|
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:
|
👋 Please assign me here i reviewed #49192 |
📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
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:
|
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:
|
Regression Test Proposal
Do we agree 👍 or 👎. |
Regression test created. Payment summary as follows:
Paid, closing! |
Follow-up to here.
Two items to add:
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:
CC: @Expensify/design @luacmartins @adamgrzybowski @ikevin127
Issue Owner
Current Issue Owner: @Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @trjExpensifyThe text was updated successfully, but these errors were encountered: