-
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
Update dynamically attachment modal #22543
Conversation
@narefyev91 Please 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] |
Reviewer Checklist
Screenshots/VideosWeb8mb.video-EhW-6bvC514d.mp4Mobile Web - Chrome8mb.video-lF7-znKLgFD6.mp4Mobile Web - Safari8mb.video-TOI-QXXyQkHZ.mp4Desktop8mb.video-Rcx-NO0KpwyH.mp4iOS8mb.video-vhR-xwaCpLo5.mp4Android8mb.video-dBO-FJ7wcQBn.mp4 |
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.
LGTM
🎀 👀 🎀 C+ reviewed
@narefyev91 We have another bug when we update dynamically with this PR that I mentioned in slack thread here https://expensify.slack.com/archives/C01GTK53T8Q/p1688999272836799?thread_ts=1688646398.596839&cid=C01GTK53T8Q Can you take a look at this. Screencast.from.10-07-2023.21_19_32.webm |
@dukenv0307 does this happen when the sender is viewing the image? |
@jasperhuangg This only happens when the user is viewing the first image of the report and from another device, the participant of the report add an attachment. |
@dukenv0307 gotcha, then it sounds like we need to fix that before this is merged. What have you tried thus far to preserve the index of the currently displayed image? |
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.
taking away my approval since it looks like this isn't ready yet
@jasperhuangg The page state is updated correctly but when we is viewing the first image and then |
@dukenv0307 maybe you can store latest viewed page in ref and when you get updates for pages - you will check if user already seen any of the image and use that stored page index/id |
@narefyev91 |
Why does this not happened for all other platforms - but only happened for web and desktop? Also seems you should not be getting to next image if this condition will be met |
And it is not empty. |
@narefyev91 It Seems the reason is in native we revert the array and when we add new attachment, |
@dukenv0307 there's no way you can store some type of index anywhere, either in this component or the a parent component, that keeps track of the currently viewed attachment? |
@jasperhuangg |
@dukenv0307 what have you tried to fix this problem? There seems to be a |
@jasperhuangg I tried to add |
@jasperhuangg I just tried to add a callback after the update state in Screencast.from.14-07-2023.07.31.36.webm |
@jasperhuangg Friendly bump. |
@narefyev91 Please help to review this PR again. |
@narefyev91 I just updated to set |
@narefyev91 Friendly Bump for reviewing |
@jasperhuangg |
@dukenv0307 @jasperhuangg sorry guys i'm on vacation - will not be able to have one more look at it for now |
merge main to update to function component |
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.
good work!
@jasperhuangg looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ 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/jasperhuangg in version: 1.3.48-0 🚀
|
@@ -50,7 +51,7 @@ function createInitialState(props) { | |||
|
|||
const page = _.findIndex(attachments, (a) => a.source === props.source); | |||
if (page === -1) { | |||
throw new Error('Attachment not found'); | |||
Navigation.dismissModal(); | |||
} |
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.
Do we need to return
after this so that props.onNavigate
doesn't still get called?
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.
@Beamanator When we create the PR, this component is a class component and I added return here. And then after it is changed to a function component, I resolve conflict, and if we return here the function doesn't return the state and the app also crashes, So I think we should the check to prevent call props.onNavigate
or check in props.onNavigate
function to do nothing if the attachment is empty.
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.
Ya looks like this is causing this regression: #23964
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.
@Beamanator Do I need to create a PR to quickly fix it?
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.
@dukenv0307 interesting - I did this and it seemed to work fine for me: #23988
So I returned an object, not undefined
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.
Thanks for offering creating a PR, but I already created one 😅 ^ feel free to review if you'd like
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.
So I returned an object, not undefined
That looks good to me.
Thanks for offering creating a PR, but I already created one sweat_smile ^ feel free to review if you'd like
Thank you.
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.
Hey @Beamanator and @dukenv0307,
I've noticed we've opted to remove the error throw in this section. While I understand this might be an attempt to address the issue of deleting an image, we need to recall that it was previously flagged as a bug when the carousel closed upon an image deletion (Issue #18451). Reintroducing this behavior might not be the best move.
The original intent of the createInitialState
—as the name suggests—was to execute just once upon the carousel's opening. Its purpose was to display the "Ooops" screen if a user tries to open an unlocatable attachment. In my opinion, presenting this error screen offers a more insightful user experience compared to abruptly closing the modal, which may come off as if the user's action was disregarded.
Can we reconsider this change and perhaps retain the error throw?
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.48-5 🚀
|
🚀 Deployed to staging by https://github.com/jasperhuangg in version: 1.3.49-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.49-3 🚀
|
Details
Update dynamically attachment modal
Fixed Issues
$ #21334
PROPOSAL: #21334 (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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Screencast.from.10-07-2023.15.46.21.webm
Screencast.from.10-07-2023.15.48.38.webm
Mobile Web - Chrome
Record_2023-07-10-15-53-05.mp4
Record_2023-07-10-15-53-33.mp4
Mobile Web - Safari
safari-1.mp4
safari-2.mp4
Desktop
desktop-1.mp4
desktop-2.mp4
iOS
ios-1.mp4
ios-2.mp4
Android
21334-1.mp4
21334-2.mp4