adding carousel to attachment modal #9279
Conversation
|
I set this branch up to get a signed commit in. I'll be looking this comment first and then I'll need to update the videos I made for the updated buttons. Also I think there's about 2 comments that I've responded to that I left unresolved, but the rest I either went with the suggested code or unchanged the code. |
|
Review paused until santhosh get's back from OOO. Adding him as reviewer so that he sees the PR once he's back. |
There was a problem hiding this comment.
Thanks for the patience @JediWattson! Left some comments please address those questions and add code your suggestions as comments. Don't commit any changes.
src/components/AttachmentCarousel.js
Outdated
| } | ||
|
|
||
| /** | ||
| * increments or decrements the index to get another selected item |
There was a problem hiding this comment.
The comment shouldn't be too technical.
There was a problem hiding this comment.
How about: Changes the attachment shown in the modal
There was a problem hiding this comment.
Ys sound better
Helps to navigate between next/previous attachments
src/components/AttachmentCarousel.js
Outdated
| this.setState((prevState) => { | ||
| const attachments = prevState.attachments; | ||
| const page = prevState.page; |
There was a problem hiding this comment.
could have just destructured attachments, page
There was a problem hiding this comment.
Got it, I was going with the idea that I couldn't destructure the state. I could just destructure the argument though?
There was a problem hiding this comment.
this.setState(({attachments, page}) => { ...
src/components/AttachmentCarousel.js
Outdated
| * @param {Boolean} shouldDecrement | ||
| */ | ||
| cycleThroughAttachments(shouldDecrement) { |
There was a problem hiding this comment.
Can we avoid boolean shouldDecrement and have some better implementation or better naming.
Ex: Could've passed -1 for back & +1 right arrow to the method then just add it to the page to get the next index
Any thoughts @chiragsalian?
There was a problem hiding this comment.
I like the idea of using calling the function with 1 and -1 here and call call the argument diff then add it?
There was a problem hiding this comment.
cycleThroughAttachments(diff) {
...
const nextIndex = page + diff;
...
}
// jsx
<Button
onPress={() => this.cycleThroughAttachments(1)} // or -1 for back
... // props
/>There was a problem hiding this comment.
yeah i would do cycleThroughAttachments with param maybe deltaSlide containing values as +1 or -1 and then do,
const newIndex = page + deltaSlide;
|
@JediWattson From issue description #7862 (comment) Expected Result:
I believe PR covers only 1 & 2 what above 3, 4, 5. |
for 3 I would propose to add a listener to handle the arrow press. It would be added when the component is mounted: componentDidMount() {
document.addEventListener('keydown', this.handleArrowPress)
}
componentWillUnmount() {
document.removeEventListener('keydown', this.handleArrowPress)
}handleArrowPress will check the e.key and call cycleThroughAttachments /**
* Listens for keyboard shortcuts and applies the action
*
* @param {Object} e
*/
handleArrowPress(e) {
if(e.key === "ArrowLeft" && !this.state.isBackDisabled) {
this.cycleThroughAttachments(-1);
}
if(e.key === "ArrowRight" && !this.state.isForwardDisabled) {
this.cycleThroughAttachments(1);
}
}
<SwipeableView>
<AttachmentView sourceURL={sourceURL} file={this.state.file} />
</SwipeableView>
// ... <AttachmentCarousel
|
From 5 we can get the answer, we should toggle arrow visibility when tapping on the image. But I'm not sure whether we should show or hide the arrows initially. @Expensify/design team please help us here. cc: @shawnborton |
|
@JediWattson Sounds good! Please make the suggested changes, let me know when changes are done. |
|
For mobile, I think it's okay if they are visible by default and then tapping the image hides them. |
|
I just got most of the changes here up. I just need to add the functionality for the arrows to show and not show |
|
@JediWattson Any update on this? |
|
I'm finishing up the arrows today, and then possibly get into the swiping too. I can add new videos showing this this weekend too. |
|
@JediWattson Further update on this? |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
@JediWattson You should use only one git account. Do not use multiple accounts. Can you revert & commit changes properly thanks! |
|
Will do, I think I've completed the requirements now too. Now it seems I just need to clean that up, and add some videos for swiping and toggling arrows |
41fc8a3 to
e2e9633
Compare
|
I believe all I have left now is to make some videos showing the arrows show and not showing along with swiping and update the qa instructions |
|
Good then,
And let us know when PR is ready for review. |
|
@JediWattson Bump! |
|
@Santhosh-Sellavel I got some new videos up to show the swiping in native apps. A couple things I really can't figure out are: in mobile browsers I'm unable to capture an event to toggle showing the arrows, and in a pdf I couldn't get a similar event so I left the arrows on there |
Sorry for the delay, are these both specific to mobile web alone? |
|
@JediWattson bump! |
|
the arrows not toggling is specific to mobile browsers, but the pdf seems to be an issue for both mobile browsers and native |
|
I believe I got this 100% now! @Santhosh-Sellavel |
| this.debouncedConfigureImageSource = _.debounce(this.configureImageSource, 220); | ||
|
|
There was a problem hiding this comment.
This is required only for attachments flow, should this debounce be set here?
As it add debounce all other views which uses this component
There was a problem hiding this comment.
It seems fine to debounce these in the image component since the logic to configure the image source is in it already.
There was a problem hiding this comment.
Hmm i don't mind the debounce but can someone remind me why we need the debounce here? Is it because if the user clicks next rapidly we want to load the image properly or is there some other reason?
I'm mostly wondering if we can trigger the debounce immediately via,
this.debouncedConfigureImageSource = _.debounce(this.configureImageSource, 220, true);
So that non-attachments are not impacted and attachments should load faster without the delay. Wouldn't that work better?
There was a problem hiding this comment.
That could work if there’s a Boolean prop that would make that false for the attachment carousel
There was a problem hiding this comment.
I tested this a bit more wondering if it's related to clicking next button rapidly or resizing and i could not reproduce either when I remove the debounce (tested on web and ios). Can we recheck if this debounce is necessary and if so maybe add a comment explaining why?
There was a problem hiding this comment.
There should be no debounce
- It's a workaround
- From your explanation the change is only concerning the carousel - everything else would not use debounce (the same way the carousel will)
I've submitted a PR for RN web to our fork. If fixes some inconsistencies with the Image component. I think it would address the need for debounce as well
There was a problem hiding this comment.
That definitely tracks. I don't know if the way I'm going about it is the same as what you mentioned, but I was digging into this more and the crux of the problem is that getSize really shouldn't be used at all. In fact I managed to refine the Image component into a few lines of code.
function Image({ source, isAuthTokenRequired, ...props }) {
if (isAuthTokenRequired) {
source.uri = addEncryptedAuthTokenToURL(source.uri);
}
// eslint-disable-next-line
return <RNImage {...props} source={source} />;
}
There's some changes I had to make in a couple other places, but not too much is really changed. I'm now using onLoad to send the height and width which changes the arguments to the parent functions. Also I needed a check in the ImageView to make sure the url was correct before setting the scale.
I was wondering if all this could be a proposal for this issue because it does reduce the loading? I just need to clean up the code a bit to make a PR for it too.
There was a problem hiding this comment.
The custom onLoad with getSize was also a workaround, because at the time react-native-web did not return image dimensions with the onLoad callback prop
Our fork of RN web has a fix for this and I'm applying it to the Image component, but the PR is blocked ATM
(we already have the fix merged to our fork, so onLoad works, but the PR is blocked due to image headers issues - auth token would be placed in headers in the future)
There was a problem hiding this comment.
@kidroca it looks like a change for removing the debounce was merged which should resolve this thread. Was there anything else to consider or is this all set??
|
We discussed this 1:1 and we think it's better to merge the PR and tackle the issues below as a follow up -- some of them might have been addressed already: |
|
✋ 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/luacmartins in version: 1.2.81-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.2.81-1 🚀
|
|
Awesome work getting this to production! I've been enjoying this one, really great improvement. |
| <View style={styles.imageModalImageCenterContainer}> | ||
| {this.state.source && this.state.shouldLoadAttachment && ( | ||
| {this.state.reportID ? ( |
There was a problem hiding this comment.
#15809 regression was caused by this PR. When user navigates to the details of a user through participants list of a group chat, the route contains the reportID and hence user avatar is displayed with <AttachmentCarousel> instead of <AttachmentView>.
|
👋 Hey all, dropping in to let you all know that tooltips weren't added for the |
|
I think we missed handling the password-protected PDF UI during this PR. Buttons are relatively big which covers the form. #15896 |
|
This issue was not handled in this PR. Image automatically changes when carousel is open and a new image is received in the chat. |
| @@ -276,14 +285,12 @@ class ImageView extends PureComponent { | |||
| > | |||
| <Image | |||
| source={{uri: this.props.url}} | |||
| isAuthTokenRequired={this.props.isAuthTokenRequired} | |||
There was a problem hiding this comment.
It seems isAuthTokenRequired this was removed by mistake - this prop should always be forwarded to the image
| style={this.state.zoomScale === 0 ? undefined : [ | ||
|
|
||
| // Hide image until finished loading to prevent showing preview with wrong dimensions. | ||
| style={this.state.isLoading ? undefined : [ |
There was a problem hiding this comment.
This minor issue was introduced by changes made in this PR. The issue is fixed by this PR by including zoomScale along with isLoading.
Details
add a carousel to the attachment modal that navigates through a chat's attachments
Fixed Issues
#7862
PROPOSAL: #7862 (comment)
Tests
iOS / android
web / desktop
Offline tests
QA Steps
iOS / android
web / desktop
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
mweb.chrome.mov
Mobile Web - Safari
mweb.safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
Screen.Recording.2022-12-30.at.2.00.40.PM.MOV