Skip to content
This repository has been archived by the owner on Jan 22, 2022. It is now read-only.

Add reader mode #517

Open
wants to merge 5 commits into
base: xenial
Choose a base branch
from
Open

Conversation

DavidVentura
Copy link

@DavidVentura DavidVentura commented Sep 9, 2021

Hi!
This PR adds "Reader mode" to morph -- the readability.js file is taken straight out of Mozilla's repo here. The style and header are the bare minimum to get something usable, they could be improved.

I have absolutely no idea if the placement of my code makes any sense -- to me the 'readerMode' state should be stored in each tab, but that's likely incorrect

Attached, a video of behavior

readability.mp4

@@ -279,5 +296,15 @@ Component {
}
}
}
function toggleReaderMode() {
if (browserTab.readerMode) {
browserTab.reload()
Copy link
Author

Choose a reason for hiding this comment

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

i thought about storing the entire dom in the makeDomReadable function, but I'm not sure it's worth the tradeoff.
I don't really understand how that'd work with something like a video that's playing (if a reference is stored in a "shadow dom", does it keep playing? etc)

@mateosalta
Copy link
Contributor

i think eventually we will switch over to the builtin blink overlay scrollbars, the css injection scrollbar theme is a bit hacky and not as nice, so probally dont worry about them too much unless you know an easy way to make an exception for that script.

@mateosalta
Copy link
Contributor

I think the placment makes sense to me - but perhaps there will be people who want less items in the navigation? with that in mind it may make sense to have a disable option in settings

@mateosalta
Copy link
Contributor

try looking at 'stock-ebook' , 'note' icons from our suru theme

@DavidVentura
Copy link
Author

stock_ebook is indeed nicer, thanks!

nicolascolla added a commit to nicolascolla/onion-surf that referenced this pull request Oct 13, 2021
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.

2 participants