Closed m-natarajan closed 4 days ago
melvin-bot[bot]
commented
3 months ago Triggered auto assignment to @OfstadC (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
melvin-bot[bot]
commented
3 months ago Triggered auto assignment to @neil-marcellini (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
github-actions[bot]
commented
3 months ago :wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
m-natarajan
commented
3 months ago We think that this bug might be related to #vip-vsb
roryabraham
commented
3 months ago cc @kidroca I think this might be related to https://github.com/Expensify/App/pull/44889
melvin-bot[bot]
commented
3 months ago Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.
roryabraham
commented
3 months ago not sure this needs to be a blocker
roryabraham
commented
3 months ago Curious for design input ... what should we display when you upload an image and then delete it, all offline? What should be the "strikethrough" equivalent for images?
dubielzyk-expensify
commented
3 months ago Good question. There's a few things we could do:
A) Delete it altogether and not show it
B) Put some sort of visual treatment over it similar to what we do with the offline thumbnail. So something like the mock to the complete right:
I'm leaning B so that we're consistent with the other offline behaviours of strikethroughs etc.
Krishna2323
commented
3 months ago Chat - File does not appear with strikethrough style when uploaded offline and deleted
We aren't handling deleted state for attachments.
In AnchorRenderer we have the parentStyle which is being used to check if the action is deleted or not, we can pass hasStrikethroughStyle to AnchorForAttachmentsOnly and then to DefaultAttachmentView.
https://github.com/Expensify/App/blob/e0f17df84a677c6a6cc412e0fa132782d5df700c/src/components/HTMLEngineProvider/HTMLRenderers/AnchorRenderer.tsx#L58-L59
In DefaultAttachmentView we can use the prop isDeleted (hasStrikethroughStyle) to apply the line through styles. isDeleted && styles.lineThrough
https://github.com/Expensify/App/blob/e0f17df84a677c6a6cc412e0fa132782d5df700c/src/components/Attachments/AttachmentView/DefaultAttachmentView/index.tsx#L43
We can also do the same for image and video renderers. In ImageRenderer we can create a variable isDeleted just like we created that for hasStrikethroughStyle in AnchorRenderer. Then we can pass isDeleted to ThumbnailImage and use that prop in ThumbnailImage to render delete icon when true.
We also need to remove isUploading check from AttachmentCommentFragment otherwise the stylings will be only applied when attachment is uploaded and deleted in offline mode.
Note: Test branch only fixes for attachments and images. We should also check for other components in AttachmentView and should also apply the change for videos if required.
https://github.com/user-attachments/assets/f84098f4-2712-4ec4-a7f2-12fb6e5ad1aa
Krishna2323
commented
3 months ago @roryabraham, I don't think this is a regression. The same can be reproduced on production, only difference is that we were showing uploading attachment... text before and now we have a preview for attachments in offline mode also.
https://github.com/user-attachments/assets/e5a59bdf-dcf0-481e-9c04-b4de21649d5a
roryabraham
commented
3 months ago I'm going to demote this to NAB
OfstadC
commented
3 months ago Reassigning as i'm out tomorrow and Monday
melvin-bot[bot]
commented
3 months ago Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
OfstadC
commented
3 months ago @kevinksullivan Feel free to assign back to me on Tuesday - I just don't want the first steps held up on me 😃
dubielzyk-expensify
commented
3 months ago Forgot to cc @shawnborton for his thoughts on this solution
shawnborton
commented
3 months ago For your B solution, I think my first reaction is it feels like a connectivity thing that the image wouldn't load.
What if we literally tried the same treatment as a strikeout? Like we made the image at 50% opacity, and then put a 1px horizontal border running through the middle with some overlap on the sides? Is that nuts?
neil-marcellini
commented
3 months ago I'm going to make Jon the owner until we nail down the design. I personally like this idea:
dubielzyk-expensify
commented
3 months ago What if we literally tried the same treatment as a strikeout? Like we made the image at 50% opacity, and then put a 1px horizontal border running through the middle with some overlap on the sides? Is that nuts?
Is this what you mean? If so, then it feels very odd. Feel free to play around in Figma
This is also a pretty edge-casey scenario. People have to be offline and delete a photo.
shawnborton
commented
3 months ago That is what I was thinking, but I wasn't expecting a dark border/negative space around the strikethrough line. Also FWIW, when we strike out text, we use our text color for the strikethrough line right? So maybe the strikethrough line/border would use our text color too and not our border color?
shawnborton
commented
3 months ago So I think this is what I was thinking:
I agree though, it does feel a bit odd...
dubielzyk-expensify
commented
3 months ago That is what I was thinking, but I wasn't expecting a dark border/negative space around the strikethrough line.
The reason for this is the strikethrough could be very hard to see in some instances so it would create some visual distinction.
I think the whole strikethrough looks a bit broken to me. Never seen this pattern to me. Keen to hear what @dannymcclain thinks though
kidroca
commented
3 months ago I'm also thinking strike makes sense for text, but isn't intuitive on images. Replacing the image with a placeholder for deleted attachments (the bin), or some kind of a blend in case users have to see the underlying original image, seems more intuitive to me.
shawnborton
commented
3 months ago I think the whole strikethrough looks a bit broken to me. Never seen this pattern to me.
Yeah, I think I agree. I was curious to see what it would look like, but after seeing it in practice maybe it's not so great.
Replacing the image with a placeholder for deleted attachments (the bin)
Definitely down to try this out. So maybe something like Jon's 4th screen here, but perhaps the trash can icon is overlaid on top of the image somehow? Not sure if that will look great or not but maybe worth trying?
dannymcclain
commented
3 months ago When the user goes back online in this situation, what happens? Do the messages just disappear completely?
I think Jon's option B + trash can mock looks good, but I'm honestly questioning why we don't just remove the message entirely if that's what is going to happen once you're back online anyways. What value is the user getting out of seeing the trashcan icon or file preview with strikethrough?
shawnborton
commented
3 months ago I guess the idea here is it's a pending action? But yeah... maybe it should just be optimistic? I am going to call upon my favorite offline pattern expert @trjExpensify for a 4th opinion here :)
dubielzyk-expensify
commented
3 months ago Yeah. I think we should keep the image there, Danny, because if not you'll see strikethrough on text for deleted text, but deleted images will go away. I think we should at least do the same everywhere for consistency.
dannymcclain
commented
3 months ago Ok yeah that makes sense to me. In that case, I definitely prefer the trash-can-icon-placeholder-thingy version. Feels very clear that there used to be an image here, but it has been deleted.
trjExpensify
commented
3 months ago I guess the idea here is it's a pending action? But yeah... maybe it should just be optimistic? I am going to call upon my favorite offline pattern expert @trjExpensify for a 4th opinion here :)
Yeah, Pattern B here is "optimistic with feedback" - so it's pending in the UI until coming back online to confirm it succeeds or fails. For pending delete actions, we use strikethrough and 50% opacity. Then on failure, revert to 100% opacity, drop the strikethrough and RBR to an error message in-line.
We keep the "data" in the UI until we confirm it has successfully been deleted, because the optimistic action could fail. I think attachment uploads fall into the pattern B bucket because it could be something sensitive or uploaded in error, so you want to know it successfully deleted. Otherwise, we'd use "Pattern A - optimistic without feedback" and optimistically remove it from the UI and not tell you if it was successful or not when you come back online.
Anyhow, I recall we came up against this before for images in the Pattern B doc way back when. The profile avatar at the time, and we decided in this ancient thread to only reduce the opacity of the image for pending delete actions. Interestingly, looks like that's offline action is now either broken in app today or purposefully changed to Pattern A.
So I think one option could be to still do that for the mainline "uploaded online, removed offline" case:
But this edge case of "upload offline and delete offline" is sticky when it comes to "attachment only" because it's already at 50% opacity pending to be added, and so it'll seem like nothing happened. We could just live with that for now, or go with the idea to put some kind of trash overlay on pending delete images in all cases. I think that could work!
Bit of a sidebar, but related.. an intuitive way (you'd think!) to handle all of the "add and delete the same thing offline" actions would be to remove it from the UI without feedback and that's that. After all, it was pending to be added, so there shouldn't be a risk of it not being deleted and being actioned on or seen by others etc. But we really don't handle taking these two actions on the same object well until we commit to fixing the "replay effect" (context). It’s a bit of a downside of the offline design to handle these edge cases, but you basically see things get re-added, and disappears again as the queued create/delete actions run sequentially:
https://github.com/user-attachments/assets/3b77c851-27f1-4627-abb9-327f200cdc58
We determined it's not worth the effort to fix that now.
dubielzyk-expensify
commented
3 months ago Ah that's really helpful context. Are you suggesting just reducing the opacity then or delete it altogether? If we just reduce the opacity it'll share the look of the uploaded-while-offline pattern which seems non-ideal?
I think this is such a small case that I don't feel super strongly about it. I guess I was mostly leaning towards consistency in this instance, but happy for someone to make the call.
shawnborton
commented
3 months ago After all, it was pending to be added, so there shouldn't be a risk of it not being deleted and being actioned on or seen by others etc
Ah yeah, this makes complete sense to me and I would be in favor of something like that.
trjExpensify
commented
3 months ago Yeah, we could Shawn, though I think the replay effect will still end up showing it again, then removing it. So it's still not great for that "upload offline and delete offline" edge case.
That aside, we still need to decide how we want to handle the more typical case of just deleting an already uploaded attachment offline. That should remain pending delete in the UI, so do we want to do this:
"attachment + text" - strikethrough and reduce the opacity of the text, only reduce the opacity of the attachment "attachment only" - only reduce the opacity of the attachment
Recognising (to Jon's point) that for that attachment only case it doesn't use strikethrough in addition to reduced opacity, and thus appears as the same pattern as "pending create" in that regard.
Or, do we want to do something for attachment thumbnails where we reduce the opacity of the preview and add a trash can overlay or something, so it's a bit more visually distinct than just reducing the opacity like pending create?
I'm not super passionate about which one.
shawnborton
commented
3 months ago I don't mind the latter option with the trash can icon (I mentioned that here), but I think the icon itself wouldn't get reduced opacity and it might use a brand color?
trjExpensify
commented
3 months ago Cool, nice! Here's a look at using brand/ice and not reducing the opacity on the icon:
I'll leave that to the experts though! 😅
shawnborton
commented
3 months ago Oh, I was thinking just to use our standard icon color there 🤣
trjExpensify
commented
3 months ago I'll leave that to the experts though
innit.
shawnborton
commented
3 months ago Innit. I can get down with what you have there, let's see what the rest of the design team thinks though.
kidroca
commented
3 months ago Hey guys, I think I might be responsible for setting this transparency idea in motion. I suggested to blend the icon with the attachment so the user has a sense of what particular attachment/photo would get deleted when they resume connectivity, but I'm not really sure if this is a requirement.
If it's not a requirement to see the particular attachment, I think it would be much more user friendly and accessible to display a placeholder image/card containing something like "This attachment is pending deletion" and the trashcan icon
For exclusively offline actions I'm on board of removing right away. Regarding the "replay effect" it has a high impact on attachments. It would be beneficial to find some solution, because it would be pointless to upload some big attachment only to deleted it the next instant. Related to the "reply effect" and skipping the upload, I will soon be working on a separate "upload queue" that would be responsible for all attachment upload actions. It's being developed mainly to allow for bigger attachments, but it might help skip the "reply effect" when it concerns attachments
dannymcclain
commented
3 months ago I think if we're going to continue displaying the attachment, we could maybe add our overlay behind the icon to give it some more distinction from the pending state. Don't feel too strongly about that though. I also think Jon's original mock of just showing the placeholder with trashcan icon works too (so not displaying the image/attachment anymore).
shawnborton
commented
3 months ago Cool, I can honestly get down with any and all of that. Happy to go with whatever the group decides :)
dubielzyk-expensify
commented
3 months ago Whichever with the icon looks good to me 👍
trjExpensify
commented
3 months ago I love the right side from Danny here: https://github.com/Expensify/App/issues/46616#issuecomment-2275957037
trjExpensify
commented
3 months ago Related to the "reply effect" and skipping the upload, I will soon be working on a separate "upload queue" that would be responsible for all attachment upload actions. It's being developed mainly to allow for bigger attachments, but it might help skip the "reply effect" when it concerns attachments
That would be great!
We're also exploring a solution for the replay effect here which might be in place as well.
melvin-bot[bot]
commented
2 months ago @kevinksullivan, @neil-marcellini, @dubielzyk-expensify Whoops! This issue is 2 days overdue. Let's get this updated quick!
melvin-bot[bot]
commented
2 months ago @kevinksullivan @neil-marcellini @dubielzyk-expensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
melvin-bot[bot]
commented
2 months ago @kevinksullivan, @neil-marcellini, @dubielzyk-expensify Huh... This is 4 days overdue. Who can take care of this?
dubielzyk-expensify
commented
2 months ago Looks like we have a solution here then. Let's get to implementing @neil-marcellini and/or @kevinksullivan 😄
neil-marcellini
commented
2 months ago Sorry for the delay. I'm getting caught up on the huge discussion here.
From Tom's longer comment:
Bit of a sidebar, but related.. an intuitive way (you'd think!) to handle all of the "add and delete the same thing offline" actions would be to remove it from the UI without feedback and that's that. After all, it was pending to be added, so there shouldn't be a risk of it not being deleted and being actioned on or seen by others etc. But we really don't handle taking these two actions on the same object well until we commit to fixing the "replay effect" (context). It’s a bit of a downside of the offline design to handle these edge cases, but you basically see things get re-added, and disappears again as the queued create/delete actions run sequentially:
I thought we fixed the replay effect! I can remember putting a month or more of effort into that so it's quite a bummer to see that it's still broken 😞. Oh well. I think we could implement what you're suggesting there regarding optimistic data although it would be challenging, and it's not directly related to the replay effect. Sounds like there are some plans to get it fixed so that's great
neil-marcellini
commented
2 months ago To be clear, we're going with the mockup on the right side here, yeah?
@Krishna2323 would you please update your proposal?
melvin-bot[bot]
commented
2 months ago Job added to Upwork: https://www.upwork.com/jobs/~01f435ef631a03057d
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.15-4 Reproducible in staging?: y Reproducible in production?: no, production has different behavior If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause internal team Slack conversation:
Action Performed:
Expected Result:
The file will appear with strikethrough style when deleted offline (production behavior).
Actual Result:
The file does not appear with strikethrough style when deleted offline.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
https://github.com/user-attachments/assets/1b7124c0-9a7d-4181-b483-a168912ff58f
View all open jobs on GitHub
Issue Owner
Current Issue Owner: @OfstadCUpwork Automation - Do Not Edit