-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Chat - File does not appear with strikethrough style when uploaded offline and deleted #47971
Chat - File does not appear with strikethrough style when uploaded offline and deleted #47971
Conversation
…ed offline and deleted. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@dubielzyk-expensify @mananjadhav One of you needs to 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] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Let me know when there's something visual to check out :) |
The code changes are done, I will test last time today and upload the screenshots. |
@dubielzyk-expensify, please check the recording and let me know if we want some changes. Monosnap.screencast.2024-08-28.05-29-20.mp4 |
Yup, I'm also curious about what Jon is mentioning. |
Same. Other than that weirdness Jon pointed out I think it's looking good. |
Thanks for the review, I think that was already deleted action which might have failed that's why it was shown but I will double check that. |
@Expensify/design, I apologize for forgetting to show the visuals in light mode 😅. Could you please confirm if the video looks correct in both modes? Monosnap.screencast.2024-08-29.06-23-50.mp4 |
Thanks for providing video for light mode 😄 Is the icon using the icon color variable? Cause it looks to be the same icon color as in dark mode which is a darker than we want. In light mode it should appear a bit lighter. But it could be the recording screwing up with me here. |
I think the video might be correct? I think the color for icons is darker in light mode, and lighter in dark mode. |
Thanks, I will update and provide the result shortly. |
@Expensify/design, does this look correct? The color code has been updated to the deleted_indicator_updated_color.mp4 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
src/components/HTMLEngineProvider/HTMLRenderers/VideoRenderer.tsx
Outdated
Show resolved
Hide resolved
src/components/Attachments/AttachmentView/DefaultAttachmentView/index.tsx
Outdated
Show resolved
Hide resolved
return ( | ||
<> | ||
<View | ||
style={[styles.pAbsolute, styles.alignItemsCenter, styles.justifyContentCenter, styles.highlightBG, styles.deletedIndicatorOverlay, styles.deletedIndicator, containerStyles]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still going through these styles.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Hi, thanks for asking. The latest I've heard is that if the ES Lint checks are related to files touched in the current PR, then we require them to be fixed before the issue can be considered complete and paid. If the fixes are small then we like to see the changes in the current PR. Otherwise, like in this case where we need to migrate to useOnyx, we should handle it in a separate PR. You could also search to see if there is already an issue to migrate to useOnyx for these files. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@mananjadhav, I have fixed other files but I'm no sure what to do with App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx Lines 117 to 126 in 7dde09e
|
@mananjadhav, friendly bump on the comment above ^ |
@mananjadhav, any updates? |
I couldn't check this over the weekend, but I'll try to check today. I think it's best to post on open-source channel to get more eyes. |
@mananjadhav, we no longer need to make this change, I guess it has been done somewhere else. You can continue with the checklist. |
Thanks for the update @Krishna2323. I will check this out today. |
Reviewer Checklist
Screenshots/VideosiOS: NativeMacOS: Chrome / Safariweb-delete-attachment.movMacOS: Desktopdesktop-delete-attachment.mov |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Did the latest review, left with the QA on mobile devices. Will finish the checklist today. |
@Krishna2323 Can you please fix conflicts? Also there's a recent node and npm change that causes the performance tests to fail. I am having some trouble with my Xcode, rest screenshots updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am having issues with iOS so skipping that one. Screenshots for the rest are uploaded and I verified iOS on the author checklist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks fine and looks like it tests well. Thank you for the hard work.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.54-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.0.54-11 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.0.54-11 🚀
|
Details
Fixed Issues
$ #46616
PROPOSAL: #46616 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Monosnap.screencast.2024-09-04.13-32-54.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.1.mp4
MacOS: Desktop
desktop_app.mp4