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

feat: add option to disable horizontal swiping when scroll is true. #624

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

tm-bookshop
Copy link

I'm looking to add the equivalent functionality that was added here: readium/swift-toolkit#531

@tm-bookshop tm-bookshop marked this pull request as ready for review March 12, 2025 22:17
Copy link
Member

@mickael-menu mickael-menu 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 contributing this @tm-bookshop!

@qnga
Copy link
Member

qnga commented Mar 21, 2025

The name scrollModeDisableSwipePagination sounds a bit confusing to me. Pagination? In scroll mode?scrollModeDisableSwipe could be enough. It's a bit mysterious but at least not confusing.

@tm-bookshop
Copy link
Author

tm-bookshop commented Mar 21, 2025

The name scrollModeDisableSwipePagination sounds a bit confusing to me. Pagination? In scroll mode?scrollModeDisableSwipe could be enough. It's a bit mysterious but at least not confusing.

Fair point @qnga , I thought about "swipe" initially but thought that was also confusing. Swipes can be up/down or left/right and this only impacts left/right. disclaimer: my understanding of this repo is not very deep.

The reason I chose "pagination" is because it seemed to fit best with "resource paging" (via the R2ViewPager).

I'm not really attached to the name though, so happy to with whatever the reviewers land on. Maybe something like "scrollModeDisableSwipeResourcePaging" if we want to get real verbose?

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