Fix - On increasing the screen size the user can see the previous image in the list #17760 #18615
Conversation
melvin-bot
bot
requested review from
aimane-chnaif and
chiragsalian
and removed request for
a team
|
@chiragsalian @aimane-chnaif 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] |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
|
I have read the CLA Document and I hereby sign the CLA |
|
@AmjedNazzal PR title should be meaningful |
AmjedNazzal
changed the title
|
@aimane-chnaif Updated the title :) |
|
@aimane-chnaif my suggested change uses -40 instead of -39 because that seem to be the originally intended spacing, also it gives the border width of 1 to small screens. |
I checked the difference from console log and -39 was the correct value |
Co-authored-by: Aimane Chnaif <96077027+aimane-chnaif@users.noreply.github.com>
| @@ -252,7 +252,8 @@ class AttachmentCarousel extends React.Component { | |||
| * @returns {JSX.Element} | |||
| */ | |||
| renderCell(props) { | |||
| const style = [props.style, styles.h100, {width: this.state.containerWidth}]; | |||
| // We are isolating the method this function uses to update the width of the container to make it respond faster | |||
| const style = [props.style, styles.h100, {width: this.props.isSmallScreenWidth ? this.props.windowWidth - 1 : this.props.windowWidth - 39}]; | |||
There was a problem hiding this comment.
For code dry, let's make 40 common value for centered modal margin and use it everywhere
There was a problem hiding this comment.
And apply same for borderWidth
|
@AmjedNazzal any update on code dry suggestion? |
|
@aimane-chnaif We already have a central modal center here: App/src/components/AttachmentCarousel/index.js Lines 110 to 116 in 1d951e6 We are indeed using it in the FlatList, problem is that I tried this: And this does give the correct styling for centered modals with -40 and borderWdith but it's still not updating fast enough to solve the issue of the browser scaling and you'd still see the previous and next attachment, I think the issue here is that the central calculation is not capable to keep up with the speed of increasing browser size fast. |
|
@AmjedNazzal I just asked dry code, not change logic |
|
@aimane-chnaif Looking at changes you made, is there a specific reason for why we can't just do this? or would you say this is changing logic? aside from that your changes look great. const modalStyles = styles.centeredModalStyles(this.props.isSmallScreenWidth);
const style = [props.style, styles.h100, modalStyles, {width: this.props.windowWidth + 1}]; |
|
@AmjedNazzal so if I rephrase your question, why can't we use |
It should be exactly the same as layout width. It will cause regression if you have many attachments and click arrow buttons to see prev/next attachment. |
|
@aimane-chnaif sure that make sense then, thank you for clarifying. I think the changes you made are as DRY as they can be since everything is now connected to one thing in styles.js |
|
@AmjedNazzal please pull from main |
|
@AmjedNazzal can you also address comments I suggested? |
Co-authored-by: Aimane Chnaif <96077027+aimane-chnaif@users.noreply.github.com>
Co-authored-by: Aimane Chnaif <96077027+aimane-chnaif@users.noreply.github.com>
|
@aimane-chnaif All done |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
|
✋ 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/chiragsalian in version: 1.3.14-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
| 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}]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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:
- Upon opening the carousel, we capture the initial values of both
containerWidthandwindowWidth. - Changes to the window size are broadcast first, allowing us to respond instantly as these changes occur.
- We then calculate the proportion by which
windowWidthhas either shrunk or expanded, and immediately apply this same proportion tocontainerWidth.
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.
There was a problem hiding this comment.
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?
Details
Addresses the issue with resizing the browser causing other images in the AttachmentCarousel to show up for a second by changing the method in which the width of the attachment container updates.
Fixed Issues
$ #17760
PROPOSAL: #17760 (comment)
Tests
Offline tests
N/A
QA Steps
Same steps as the test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-05-09.at.12.05.26.AM.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
Screen.Recording.2023-05-09.at.1.40.52.AM.mov
iOS
Android