Skip to content

LUN-130: Zoom in photo with react-native-image-zoom-viewer - #128

Merged
ztefanie merged 13 commits into
mainfrom
LUN-130-zoom-in-photo-with-react-native-image-zoom-viewer
Sep 1, 2021
Merged

LUN-130: Zoom in photo with react-native-image-zoom-viewer#128
ztefanie merged 13 commits into
mainfrom
LUN-130-zoom-in-photo-with-react-native-image-zoom-viewer

Conversation

@ztefanie

Copy link
Copy Markdown
Member

This pull request belongs to an issue on our bugtracker.
You can find it there by looking for an issue with the key which is mentioned in the title of this pull request.
It starts with the keyword LUN.

@ztefanie
ztefanie force-pushed the LUN-130-zoom-in-photo-with-react-native-image-zoom-viewer branch from e1cf1ec to 3850fdb Compare August 11, 2021 11:50
@ztefanie
ztefanie force-pushed the LUN-130-zoom-in-photo-with-react-native-image-zoom-viewer branch 2 times, most recently from a6569aa to 59ecd7b Compare August 23, 2021 12:14
@ztefanie
ztefanie force-pushed the LUN-130-zoom-in-photo-with-react-native-image-zoom-viewer branch from 59ecd7b to 41620f7 Compare August 23, 2021 12:17
@ztefanie
ztefanie force-pushed the LUN-130-zoom-in-photo-with-react-native-image-zoom-viewer branch from 8e70c4c to a038993 Compare August 23, 2021 13:08
@ztefanie

Copy link
Copy Markdown
Member Author

@Taggotty @klinzo

I have a problem with this feature and would appreciate if you can help me, as i tried to fix it for hours and are out of ideas.

Problem is: If going to VocabularyList and click on a item the modal opens, but has several bugs when using the go-to-next-word-button.
a) If opening a word with two images, swipe in the carousel to the second image and click the next-word-button, the currentIndex of the carousel is not resettet and no image is displayed
b) If opening the modal of a word with only one image and click on go-next-button and this word has to images, the second image is not displayed in the carousel.

I added a video of the Problem

output.mp4

@steffenkleinle

Copy link
Copy Markdown
Member

I can't view your video.

Seems like this is not really supported but there is an open PR:
ascoders/react-native-image-viewer#477

A relatively simple workaround would be adding a key property that changes whenever imageUrls updates. A (maybe too) simple version would be to just use the id of the first image in the array, I guess it should change whenever imageUrls if always the whole array is replaced.

      <ImageViewer
        key={images[0].id}
        imageUrls={imagesUrls}
        renderImage={renderItem}
        renderIndicator={renderIndicator}
        backgroundColor={COLORS.lunesWhite}
      />

A more robust solution would probably be to somehow hash/add all image urls together, perhaps by concatenating all stringified image ids.

@ztefanie
ztefanie marked this pull request as ready for review August 30, 2021 08:16

@sarahsporck sarahsporck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I personally think the caroussel is sometimes barely visible. Maybe it is better to display the dots below the images. But that can also be an issue for later.

  • Tested on Android Emulator

Comment thread src/components/ImageCarousel.tsx Outdated
Comment thread src/components/ImageCarousel.tsx Outdated
Comment thread src/routes/vocabulary-list/components/__tests__/VocabularyListModal.spec.tsx Outdated

@steffenkleinle steffenkleinle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested on Android

Comment thread src/routes/vocabulary-list/components/VocabularyListItem.tsx Outdated
@ztefanie
ztefanie merged commit 29ba1ac into main Sep 1, 2021
@ztefanie
ztefanie deleted the LUN-130-zoom-in-photo-with-react-native-image-zoom-viewer branch September 1, 2021 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants