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

Reader Pageination #733

Merged
merged 17 commits into from
Jan 14, 2024
Merged

Conversation

CD-Z
Copy link
Contributor

@CD-Z CD-Z commented Aug 11, 2023

working:

  • Swapping back and forth through pages
  • displaying next chapter button on the last page
  • Resume progress

not working:

  • Resume progress after switching reading mode
Screen_Recording_20230811_215957_LNReader.mp4

@CD-Z CD-Z marked this pull request as ready for review August 15, 2023 14:25

Choose a reason for hiding this comment

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

Are these changes expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I found some problems after converting the PR from the draft and forgot to convert it back into a draft. I haven't removed most of these yet because I wanted to look it over again but didn't have the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And about package-lock I don't really know what is wrong with it on my PC. I always have to delete it and reinstall all packages since something doesn't work with the linking of expo-core.

const scrollTo = useCallback(
offsetY => {
offset => {
console.log('offset', offset);

Choose a reason for hiding this comment

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

Suggested change
console.log('offset', offset);

Comment on lines 356 to 359
const scrollToSavedProgress = useCallback(() => {
console.log(position);
scrollTo(position?.position);
}, []);

Choose a reason for hiding this comment

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

Suggested change
const scrollToSavedProgress = useCallback(() => {
console.log(position);
scrollTo(position?.position);
}, []);
const scrollToSavedProgress = useCallback(
() => scrollTo(position?.position),
[],
);

verticalSeekbar: boolean;
readerPages: boolean;
percentage?: number;
scrollTo: (offset: number, axis: 'Y' | 'X') => void;

Choose a reason for hiding this comment

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

Is axis being used?

theme,
minScroll = 0,
minScroll = 1,

Choose a reason for hiding this comment

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

Since we already have this readerPages ? 1 : Math.round(minScroll).

Suggested change
minScroll = 1,
minScroll = 0,

@@ -121,92 +129,26 @@ const WebViewReader: React.FC<WebViewReaderProps> = props => {
minScroll.current = (layoutHeight / contentHeight) * 100;
}
break;
case 'pages':
console.log(event.data);

Choose a reason for hiding this comment

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

Suggested change
console.log(event.data);

@@ -95,6 +95,7 @@
"showBatteryAndTime": "Show battery and time",
"showProgressPercentage": "Show progress percentage",
"swipeGestures": "Swipe left or right to navigate between chapters",
"readerPages": "Tap left or right to navigate through pages in the reader",

Choose a reason for hiding this comment

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

We could add this till all the not working stuff is implemented.

Suggested change
"readerPages": "Tap left or right to navigate through pages in the reader",
"readerPages": "Tap left or right to navigate through pages in the reader (Experimental)",

@CD-Z
Copy link
Contributor Author

CD-Z commented Jan 10, 2024

@rajarsheechatterjee Mind taking another look at it. Found some time to update the PR. I have been using this feature quite a bit and it works pretty well.

@rajarsheechatterjee
Copy link
Member

@CD-Z I'll review it this weekend. Could you rebase the PR if possible? All the other commits are also present in the git diff which would make it harder to review.

@CD-Z
Copy link
Contributor Author

CD-Z commented Jan 11, 2024

@rajarsheechatterjee No problem, fixed the rebase.

Comment on lines +114 to +119
#right, #middle {
position: absolute;
height: 100%;
width: 35%;
top: 0;
z-index: 20
Copy link
Member

Choose a reason for hiding this comment

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

user will not be able to select text from reader, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but they can't now either.

@nyagami
Copy link
Member

nyagami commented Jan 13, 2024

also, you should make a function to convert horizontal page index to vertical offset instead of using the offset directly

@rajarsheechatterjee rajarsheechatterjee merged commit 36cb7fc into LNReader:main Jan 14, 2024
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants