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

Fix - On increasing the screen size the user can see the previous image in the list #17760 #18615

Merged
merged 9 commits into from
May 12, 2023
Merged

Fix - On increasing the screen size the user can see the previous image in the list #17760 #18615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1014d69
Issue17760
AmjedNazzal May 9, 2023
487588c
Update src/components/AttachmentCarousel/index.js
AmjedNazzal May 10, 2023
b1bbbf8
Common value for centered modals
AmjedNazzal May 11, 2023
5d75473
Merge branch 'Expensify:main' into Issue17760
AmjedNazzal May 11, 2023
08ab529
Merge branch 'Expensify:main' into Issue17760
AmjedNazzal May 11, 2023
bd4858e
Update src/components/AttachmentCarousel/index.js
AmjedNazzal May 11, 2023
30d5628
Update src/components/AttachmentCarousel/index.js
AmjedNazzal May 11, 2023
f9dffc5
Merge branch 'Expensify:main' into Issue17760
AmjedNazzal May 11, 2023
c845f79
Merge branch 'Expensify:main' into Issue17760
AmjedNazzal May 11, 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
5 changes: 4 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 @@ -242,7 +242,10 @@ class AttachmentCarousel extends React.Component {
* @returns {JSX.Element}
*/
renderCell(props) {
const style = [props.style, styles.h100, {width: this.state.containerWidth}];
// Use window width instead of layout width to address the issue in https://github.com/Expensify/App/issues/17760
// considering horizontal margin and border width in centered modal
const modalStyles = styles.centeredModalStyles(this.props.isSmallScreenWidth);
const style = [props.style, styles.h100, {width: this.props.windowWidth - (modalStyles.marginHorizontal + modalStyles.borderWidth) * 2 + 1}];
Comment on lines -245 to +248
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 the ping @aimane-chnaif

As a developer, I have concerns with this approach as it seems to create an implicit coupling between components.

If, for any reason, someone changes the generic modal dimensions through methods other than marginHorizontal or borderWidth, they could inadvertently disrupt the width utilized here without even realizing it.

Moreover, the Carousel appears to be aware of its rendering context within a centered modal - a detail which, ideally, it should not be concerned with.

It seems we might be trading one bug, that only occurs on resize, for another which could potentially occur all the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kidroca thanks for the feedback. I know this is not ideal solution.
Do you have better solution to be synced with window size considering margin and border?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If addressing this bug is crucial, my preference would be to continue using containerWidth, but dynamically adjust its value in accordance with changes in window size. Here's a step-by-step outline of how this could work:

  1. Upon opening the carousel, we capture the initial values of both containerWidth and windowWidth.
  2. Changes to the window size are broadcast first, allowing us to respond instantly as these changes occur.
  3. We then calculate the proportion by which windowWidth has either shrunk or expanded, and immediately apply this same proportion to containerWidth.

This solution is universal and does not depend on specifics. It remains effective regardless of specific properties or the modal context, thereby reducing the risk of inadvertently introducing new issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. So containerWidth's first value comes from onLayout callback and we store size difference between containerWidth and windowWidth. And then whenever windowWidth changes, containerWidth = windowWidth - stored diff value. Right?


return (
<View
Expand Down
13 changes: 9 additions & 4 deletions src/styles/getModalStyles/getBaseModalStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import CONST from '../../CONST';
import variables from '../variables';
import themeColors from '../themes/default';
import styles from '../styles';


const getCenteredModalStyles = (windowWidth, isSmallScreenWidth) => ({
borderWidth: styles.centeredModalStyles(isSmallScreenWidth).borderWidth,
width: isSmallScreenWidth ? '100%' : windowWidth - styles.centeredModalStyles(isSmallScreenWidth).marginHorizontal * 2,
});

export default (type, windowDimensions, popoverAnchorPosition = {}, innerContainerStyle = {}, outerStyle = {}) => {
const {isSmallScreenWidth, windowWidth} = windowDimensions;
Expand Down Expand Up @@ -76,9 +83,8 @@ export default (type, windowDimensions, popoverAnchorPosition = {}, innerContain
marginTop: isSmallScreenWidth ? 0 : 20,
marginBottom: isSmallScreenWidth ? 0 : 20,
borderRadius: isSmallScreenWidth ? 0 : 12,
borderWidth: isSmallScreenWidth ? 1 : 0,
overflow: 'hidden',
width: isSmallScreenWidth ? '100%' : windowWidth - 40,
...getCenteredModalStyles(windowWidth, isSmallScreenWidth),
};

// Allow this modal to be dismissed with a swipe down or swipe right
Expand Down Expand Up @@ -112,9 +118,8 @@ export default (type, windowDimensions, popoverAnchorPosition = {}, innerContain
marginTop: isSmallScreenWidth ? 0 : 20,
marginBottom: isSmallScreenWidth ? 0 : 20,
borderRadius: isSmallScreenWidth ? 0 : 12,
borderWidth: isSmallScreenWidth ? 1 : 0,
overflow: 'hidden',
width: isSmallScreenWidth ? '100%' : windowWidth - 40,
...getCenteredModalStyles(windowWidth, isSmallScreenWidth),
};
swipeDirection = undefined;
animationIn = isSmallScreenWidth ? 'slideInRight' : 'fadeIn';
Expand Down
5 changes: 5 additions & 0 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1977,6 +1977,11 @@ const styles = {
backgroundColor: themeColors.modalBackdrop,
},

centeredModalStyles: (isSmallScreenWidth) => ({
borderWidth: isSmallScreenWidth ? 1 : 0,
marginHorizontal: isSmallScreenWidth ? 0 : 20,
}),

imageModalImageCenterContainer: {
alignItems: 'center',
flex: 1,
Expand Down