Attachment modal Arrow to cycle attachments in chat #9177
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
@JediWattson Can you test in all the platforms listed in PR Checklist. Also upload screen recordings for all the platforms
src/components/AttachmentCarousel.js
Outdated
| /** sourceUrl to determine the initial index of the attachment rendered in it's respective actions */ | ||
| sourceURL: PropTypes.string, | ||
|
|
||
| /** Object of report actions for this report */ | ||
| reportActions: PropTypes.objectOf(PropTypes.shape(reportActionPropTypes)), | ||
|
|
||
| /** callback from the parent to change the name and sourceUrl of parent's state */ | ||
| onArrowPress: PropTypes.func, | ||
| }; |
There was a problem hiding this comment.
Can you add meaningful comments to sourceURL & onArrowPress without being too technical
src/components/ImageView/index.js
Outdated
| componentDidUpdate() { | ||
| if (this.canUseTouchScreen) { | ||
| return; | ||
| } | ||
| Image.getSize(this.props.url, (width, height) => { | ||
| this.setImageRegion(width, height); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why this is necessary?
Can you elaborate?
There was a problem hiding this comment.
I believe so, I noticed in iOS when going through images if one image starts with a certain zoom compared to the others, then this will persist since nothing is handling the change of source in this regard.
There was a problem hiding this comment.
This is what I'm getting without that code. You'll see there's an image zoomed in and the sides are cut off.
Screen.Recording.2022-05-31.at.6.28.12.PM.mov
@JediWattson Sign the CLA Document, You can sign the CLA by just posting a Pull Request Comment same as the below format.
|
|
@shawnborton Can you confirm does it meet the design requirements |
|
@JediWattson Can you update the remaining platform videos too? |
|
Also @JediWattson I just now noticed most of your commits are unverified & have committed from two different accounts. I'm not sure why. It's recommended to use one account for all purposes cc: @chiragsalian |
b96b842 to
af6aeca
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi @JediWattson, so yes as santhosh suggested its best to use one account for all purposes. That way its easier to track the work done and lesser effort on your side to manage gpg keys for two accounts to ensure commits are signed. And speaking of signed commits, all commits have to be signed for us to accept your work. At this point you can combine all commits for this PR or open another new PR with a signed commits. Let us know which works better for you and you can check out #expensify-open-source or ask there if you are stuck as you'll see more people have faced the same issue. |
e4bb56c to
eb78fe1
Compare
|
Just to confirm the arrow button styles:
I also want to confirm that on mobile, when you tap once on the image, the buttons will be hidden. It also looks like the buttons are not rounded on iOS. |
|
Also @Santhosh-Sellavel can you be sure to ping the @expensify/design team for these kinds of reviews in the future and not just me? Thank you! |
|
Ya sure @shawnborton. But need a clarification here. Since you (along with megan who is unavailable) were involved in the discussion of the solution for this issue or assigned for design label in the issue. But is it okay pinging design team for any case ignoring design label assigned member who has more context? |
|
I would say ping both - directly ping the person who was involved in the design and then cc |
|
@JediWattson Your changes are lost in the PR, can you update and commit them back. Also, address the concerns from Shawn's comment -> #9177 (comment) cc: @chiragsalian |
|
@JediWattson cc: @chiragsalian |
eb78fe1 to
29f07d2
Compare
Details
Add an option to cycle between media when opened via buttons.
Fixed Issues
$ #7862
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly 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)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly 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)QA Steps
Screenshots
Web
Screen.Recording.2022-05-26.at.9.54.12.PM.mov
Mobile Web
chrome
mweb.chrome.mov
safari
Screen.Recording.2022-05-31.at.11.26.25.PM.mov
Desktop
Screen.Recording.2022-05-26.at.9.49.57.PM.mov
iOS
Screen.Recording.2022-05-26.at.10.00.05.PM.mov
Android
android.mov