Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Image gallery moves to older images when clicking on the right arrow button #32106

Merged
merged 27 commits into from
Jun 6, 2024

Conversation

matheusbsilva137
Copy link
Member

@matheusbsilva137 matheusbsilva137 commented Apr 2, 2024

Proposed changes (including videos or screenshots)

  • Invert the image gallery's direction so that older images are displayed when clicking on the left arrow button;
  • Add e2e tests for the image gallery.

Issue(s)

Steps to test or reproduce

I suggesting using more than 5 images to test so that pagination is applied.

Before:

old-image-gallery.mp4

After:

improved-image-gallery.mp4

Further comments

SUP-419

Copy link

changeset-bot bot commented Apr 2, 2024

🦋 Changeset detected

Latest commit: 2a9b750

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.38%. Comparing base (abebb06) to head (2a9b750).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop   #32106   +/-   ##
========================================
  Coverage    56.37%   56.38%           
========================================
  Files         2437     2433    -4     
  Lines        53770    53724   -46     
  Branches     11082    11075    -7     
========================================
- Hits         30315    30294   -21     
+ Misses       20814    20792   -22     
+ Partials      2641     2638    -3     
Flag Coverage Δ
e2e 56.07% <0.00%> (+<0.01%) ⬆️
unit 72.21% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@matheusbsilva137 matheusbsilva137 added this to the 7.0 milestone Apr 2, 2024
@matheusbsilva137 matheusbsilva137 marked this pull request as ready for review April 2, 2024 14:40
@matheusbsilva137 matheusbsilva137 requested a review from a team as a code owner April 2, 2024 14:40
Copy link
Contributor

dionisio-bot bot commented Apr 9, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@ggazzo ggazzo modified the milestones: 7.0, 6.8 Apr 9, 2024
gabriellsh
gabriellsh previously approved these changes Apr 11, 2024
Copy link
Member Author

@matheusbsilva137 matheusbsilva137 left a comment

Choose a reason for hiding this comment

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

FYI I decided to keep the e2e tests serial since the beforeAll process is heavy. It takes more time than the actual tests, so I can't see any real benefits on making the e2e tests parallel in this case
cc @gabriellsh @tiagoevanp

@scuciatto scuciatto removed this from the 6.9 milestone May 16, 2024
Copy link
Member Author

@matheusbsilva137 matheusbsilva137 left a comment

Choose a reason for hiding this comment

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

The direction: 'rtl' which I had to use to fix animations' direction is causing images to take longer to load for some reason. That's due to an issue on the swiper component (which has been reported here, but it is yet to be fixed)
So I decided to work on another approach (inverting the array instead of using the direction prop) so that we can have animations working properly. Can you please let me know what you think of this new approach? @gabriellsh @MarcosSpessatto

@MarcosSpessatto
Copy link
Member

The direction: 'rtl' which I had to use to fix animations' direction is causing images to take longer to load for some reason. That's due to an issue on the swiper component (which has been reported here, but it is yet to be fixed) So I decided to work on another approach (inverting the array instead of using the direction prop) so that we can have animations working properly. Can you please let me know what you think of this new approach? @gabriellsh @MarcosSpessatto

It looks like the issue has been closed and the developers say the bug is not with the lib itself. But since you already provided a solution by reversing the array, LGTM.

@matheusbsilva137 matheusbsilva137 added this to the 6.10 milestone May 24, 2024
@scuciatto scuciatto added the stat: QA assured Means it has been tested and approved by a company insider label Jun 6, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 6, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Jun 6, 2024
@scuciatto scuciatto added stat: QA assured Means it has been tested and approved by a company insider and removed stat: QA assured Means it has been tested and approved by a company insider labels Jun 6, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 6, 2024
@kodiakhq kodiakhq bot merged commit 04b0a76 into develop Jun 6, 2024
47 of 48 checks passed
@kodiakhq kodiakhq bot deleted the fix/image-gallery-direction branch June 6, 2024 21:08
This was referenced Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad: core stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants