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

Feature/chapter drawer #485

Merged
merged 29 commits into from
Dec 10, 2022
Merged

Conversation

CD-Z
Copy link
Collaborator

@CD-Z CD-Z commented Dec 4, 2022

Information

This feature adds a drawer to the ReaderScreen which allows the user to see chapters and switch between them.

chapter drawer - Dark Theme chapter drawer - Light Theme

Additional package

Problem

  • If Render in HTML is activated, it is not possible to swipe the drawer open, because it doesn't register the swipe on the WebView.
    • Through changing the padding-left of the html body to a marginLeft of the WebView, it creates a small part where the drawer can be opened.

@SkillGG
Copy link
Collaborator

SkillGG commented Dec 4, 2022

Wouldn't activating it by clicking a button be better? Because swiping is reserved for changing to next/previous chapter?

@CD-Z
Copy link
Collaborator Author

CD-Z commented Dec 4, 2022

I thought the same, however the swipe gesture of the drawer superimpose over the one of the gesture recognizer and since it is limited to 60 on the left side it is easy to differentiate between the both of them.
The only problem is with the WebView since there is only a very small space where you can drag the drawer.

As for why I didn't use a button. I simply haven't found a place I find suitable and think it is more intuitive to swipe.

@SkillGG
Copy link
Collaborator

SkillGG commented Dec 4, 2022

What about there:

IMG_20221204_221825.jpgIMG_20221204_221810.jpg

@SkillGG
Copy link
Collaborator

SkillGG commented Dec 4, 2022

Also adding a hard margin to WebView to allow for that swiping gesture makes the text off-center

Edit:
And sorry for being s bit nitpicky, but why a drawer?
Wouldn't a pop-up (like Tachiyomi one) be better? No issues with swiping at all and yes, you wouldn't be able to get the pop-up to show with a gesture but with a button, but it'd way simpler to implement but still have exactly the same functionality.
But maybe I think that just because I use Android Gestures so I have problem with any app overwriting them for drawers.
Maybe if you added a FAB that you could catch to open a drawer instead of doing a "anywhere on the left side just swipe" type of thing...

@CD-Z
Copy link
Collaborator Author

CD-Z commented Dec 4, 2022

No problem. I welcome other opinions and I can't complain to you since I myself am also very nitpicky sometimes. I changed it as yous suggested and left the button as a permanent in there. The setting to enable the drawer now works as a swipeGesture enabler. I really like to be able to swipe to open the drawer, so I really want to let it in there.
In my opinion a FAB isn't really suitable, since it is to distracting while reading.

src/screens/reader/ReaderScreen.js Outdated Show resolved Hide resolved
src/screens/novel/NovelScreen.js Outdated Show resolved Hide resolved
src/screens/reader/components/ChapterDrawer.js Outdated Show resolved Hide resolved
src/redux/settings/settings.reducer.js Outdated Show resolved Hide resolved
@CD-Z
Copy link
Collaborator Author

CD-Z commented Dec 5, 2022

In the latest commits I added a ts file, which helps to pass props for navigation.navigate.
I would appreciate feedback if this makes sense or is just unnecessary. Also is the placement of the file good?

@rajarsheechatterjee
Copy link
Member

In the latest commits I added a ts file, which helps to pass props for navigation.navigate. I would appreciate feedback if this makes sense or is just unnecessary. Also is the placement of the file good?

Yeah, it makes sense to define it at a single place than creating the params object everywhere.

@CD-Z CD-Z marked this pull request as draft December 7, 2022 16:03
@CD-Z CD-Z marked this pull request as ready for review December 8, 2022 20:21
@rajarsheechatterjee rajarsheechatterjee merged commit 599224a into LNReader:main Dec 10, 2022
@CD-Z CD-Z deleted the feature/chapter_Drawer branch February 18, 2023 17:17
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2023
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