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: #767 . #788

Merged
merged 8 commits into from
Feb 27, 2024
Merged

FIX: #767 . #788

merged 8 commits into from
Feb 27, 2024

Conversation

hunxjunedo
Copy link
Contributor

fixed issue #767. Fixed to bug of left/right navigators only allowing to scroll to first and last image in RTL mode. Now both nav buttons allow slide in the correct direction.

Copy link
Owner

@xiaolin xiaolin left a comment

Choose a reason for hiding this comment

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

Thanks for helping fix #767, once you address the comments I can test and verify on my end. 🙏

example/App.jsx Outdated Show resolved Hide resolved
src/components/ImageGallery.jsx Outdated Show resolved Hide resolved
src/components/ImageGallery.jsx Outdated Show resolved Hide resolved
@hunxjunedo
Copy link
Contributor Author

Noted 👍

@hunxjunedo
Copy link
Contributor Author

Applied the changes 👍

const { infinite } = this.props;
return infinite || this.canSlidePrevious();
const { infinite, isRTL } = this.props;
//so basically is the list is right to left (isRTL), the logic is inversed
Copy link
Owner

Choose a reason for hiding this comment

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

grammar fix here:

// When isRTL is true, the logic is reversed

Copy link
Owner

@xiaolin xiaolin Feb 23, 2024

Choose a reason for hiding this comment

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

Personally, I would rather have the comment removed because the code you've provided is self-explanatory. But since you left a comment, it will alert a contributor to read it, so it should be clear and concise.

Copy link
Contributor Author

@hunxjunedo hunxjunedo Feb 23, 2024

Choose a reason for hiding this comment

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

alright. A side note, it's my first time contributing to kindly excuse any inconvenience caused.

Copy link
Owner

Choose a reason for hiding this comment

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

No inconvenience at all, this is part of the code review process. Thank you for contributing!

@xiaolin xiaolin merged commit 4f81d93 into xiaolin:master Feb 27, 2024
2 checks passed
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.

2 participants