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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Jul 28, 2023
Merged

Update dynamically attachment modal #22543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f83a9b3
update attachment modal dynamically
dukenv0307 Jul 10, 2023
4dd20d4
refactor condition
dukenv0307 Jul 10, 2023
0aa8469
fix/21334
dukenv0307 Jul 10, 2023
32c3eb7
Merge branch 'main' into fix/21334
dukenv0307 Jul 12, 2023
10517e8
fix bug
dukenv0307 Jul 15, 2023
1e58e5b
format code
dukenv0307 Jul 19, 2023
a25661f
Merge branch 'main' into fix/21334
dukenv0307 Jul 19, 2023
ee6af34
fix lint
dukenv0307 Jul 19, 2023
402f106
return when dissmiss the modal
dukenv0307 Jul 19, 2023
0109bb3
fix prettier
dukenv0307 Jul 19, 2023
acf0547
aviod animated in native when dynamic update
dukenv0307 Jul 24, 2023
cda6d8d
remove comment line
dukenv0307 Jul 24, 2023
570047c
merge main
dukenv0307 Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/components/AttachmentCarousel/createInitialState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {Parser as HtmlParser} from 'htmlparser2';
import * as DeviceCapabilities from '../../libs/DeviceCapabilities';
import * as ReportActionsUtils from '../../libs/ReportActionsUtils';
import tryResolveUrlFromApiRoot from '../../libs/tryResolveUrlFromApiRoot';
import Navigation from '../../libs/Navigation/Navigation';
import CONST from '../../CONST';

/**
Expand Down Expand Up @@ -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();
}
Copy link
Contributor

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?

Copy link
Contributor Author

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.

image

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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?


// Update the parent modal's state with the source and name from the mapped attachments
Expand Down
10 changes: 9 additions & 1 deletion src/components/AttachmentCarousel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,15 @@ function AttachmentCarousel(props) {
setPage(initialState.page);
setAttachments(initialState.attachments);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
}, [props.reportActions]);

useEffect(() => {
if (!scrollRef || !scrollRef.current) {
return;
}
scrollRef.current.scrollToIndex({index: page, animated: false});
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [attachments]);

/**
* Calculate items layout information to optimize scrolling performance
Expand Down