Simplify native attachment gallery paging context and improve code #34080
Conversation
…ispader/simplify-native-attachment-carousel
|
@francoisl Please 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] |
chrispader
changed the title
…ispader/simplify-native-attachment-carousel
…ispader/simplify-native-attachment-carousel
chrispader
changed the title
|
@rushatgabhane Do you have time to review this one and complete the checklist? If not, we can find another volunteer |
src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPagerContext.ts
Outdated
Show resolved
Hide resolved
src/components/Attachments/AttachmentCarousel/Pager/AttachmentCarouselPagerContext.ts
Show resolved
Hide resolved
|
I don't have any ideas why |
This is fixed now. It will be gone after pulling main |
| /** | ||
| * The attachments to be rendered in the pager. | ||
| */ |
There was a problem hiding this comment.
There is a mix of comment styles in this PR. The most consistent pattern across the codebase to use is to keep it all on a single like like /** blah blah */. Can you please go through this PR and make sure all the prop comments match this style?
| /** | ||
| * A callback that can be used to toggle the attachment carousel arrows, when the scale of the image changes. | ||
| * @param showArrows If set, it will show/hide the arrows. If not set, it will toggle the arrows. | ||
| */ |
There was a problem hiding this comment.
FYI this is fine as a multi-line comment since it has a param description.
| /** | ||
| * The pager uses the source index and current active state to render the pages. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * The pager uses the source index and current active state to render the pages. | |
| */ |
This isn't a valuable comment IMO. The code is clear enough.
There was a problem hiding this comment.
As a general rule of thumb, code comments should explain why the logic is what it is, and comments that only explain what the code is doing are low-value.
| @@ -121,26 +147,38 @@ function AttachmentCarouselPager({items, renderItem, initialIndex, onPageSelecte | |||
| [], | |||
| ); | |||
|
|
|||
| const Content = useMemo( | |||
There was a problem hiding this comment.
Rename this to something more descriptive (and lower case the variable name). Suggestion: carouselItems.
There was a problem hiding this comment.
Why is useMemo() being used here? Is there a problem with rendering the content inline down below like it was before?
| /> | ||
| </View> | ||
| )), | ||
| [activePageIndex, activeSource, items, styles.flex1], |
There was a problem hiding this comment.
NAB: styles.flex1 will never change so it doesn't make sense to be used with useMemo()
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| .onTouchesDown((_evt, state) => { |
|
@tgolen I addressed all issues, would you mind to review it again? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Oops, I made a mistake and merged this too early without the checklist being done and without all the platform videos added for testing. I will need to revert this, and then @kirillzyusko can you create a re-revert PR so that we can get it reviewed properly? Sorry about that, folks. |
Yes, sure, let's merge a reverted PR and then I'll create a new PR. |
|
Revert has been merged! |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 1.4.42-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.42-5 🚀
|
maybe @tgolen, @pecanoro
Details
Based on #33756. Check this for actual diff.
This PR removes unnecessary prop-drilling and improves how we use the
AttachmentCarouselPagerContext.It also improves many of the components used in the (native) attachment gallery and simplifies some rendering logic.
The
MultiGestureCanvascomponent is becoming more generic and can therefore be used in other cases more easily. TheLightboxcomponent takes control of the adapted behaviour while in an attachment carousel.Fixed Issues
$ #36034
PROPOSAL:
Tests
Offline tests
None needed.
QA Steps
Same as in Tests.
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.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
Android: Native
android.webm
Android: mWeb Chrome
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-08.at.09.54.30.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop