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

[457] - Immersive reading view #771

Merged
merged 8 commits into from
May 24, 2024

Conversation

TheRealAnt
Copy link
Contributor

@TheRealAnt TheRealAnt commented May 20, 2024

Fixes #457

  • Hiding bars when scrolling down WebView & showing when scrolling up.
  • Showing bars when device rotates into landscape mode.

Demo:

Demo.1.mov

- Hiding bars when scrolling down WebView & showing when scrolling up.
- Showing bars when device rotates into landscape mode.
@TheRealAnt TheRealAnt force-pushed the 457-full-screen-reading-mode branch from 5fa9727 to ae374b0 Compare May 21, 2024 08:56
@kelson42
Copy link
Contributor

It‘s important to check that his fox has no unwanted side effect on macOS (iPadOS?)

@kelson42
Copy link
Contributor

@TheRealAnt I like that the feature is implemented with so few lines of code

@BPerlakiH
Copy link
Collaborator

@TheRealAnt Currently your changes are limited to iOS / iPad OS only, so they have no effect on macOS, which is good.
There's one thing I noticed immediately, that it also applies the bar hiding on iPad OS, which is something we did not wanted.
On iPad there are multiple things that can affect the amount of scroll needed to display the content: side bar collapsing, split screen mode and so on.
My suggestion is to keep these changes limited to iOS only for now. iPad screens are already large enough, plus as we discussed earlier, there's only 1 bar on iPad:

Simulator Screenshot - iPad (10th generation) - 2024-05-22 at 00 32 41

- Added WebView to Log.
- Added TargetDevice enum to easily check for the current device type.
Comment on lines 17 to 44
import UIKit

enum TargetDevice {
case nativeMac
case iPad
case iPhone
case iWatch

public static var currentDevice: Self {
var currentDeviceModel = UIDevice.current.model
#if targetEnvironment(macCatalyst)
currentDeviceModel = "nativeMac"
#elseif os(watchOS)
currentDeviceModel = "watchOS"
#endif

if currentDeviceModel.starts(with: "iPhone") {
return .iPhone
}
if currentDeviceModel.starts(with: "iPad") {
return .iPad
}
if currentDeviceModel.starts(with: "watchOS") {
return .iWatch
}
return .nativeMac
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't compile for macOS.
We do not use macCatalyst, and UIKit cannot be imported for macOS.
We do not target watchOS at all, so that is not needed.
I would suggest something like this instead:

Suggested change
import UIKit
enum TargetDevice {
case nativeMac
case iPad
case iPhone
case iWatch
public static var currentDevice: Self {
var currentDeviceModel = UIDevice.current.model
#if targetEnvironment(macCatalyst)
currentDeviceModel = "nativeMac"
#elseif os(watchOS)
currentDeviceModel = "watchOS"
#endif
if currentDeviceModel.starts(with: "iPhone") {
return .iPhone
}
if currentDeviceModel.starts(with: "iPad") {
return .iPad
}
if currentDeviceModel.starts(with: "watchOS") {
return .iWatch
}
return .nativeMac
}
}
#if os(iOS)
import UIKit
#endif
enum Device {
case mac
case iPhone
case iPad
public static var current: Self {
#if os(macOS)
mac
#else
switch UIDevice.current.userInterfaceIdiom {
case .pad: return iPad
case .phone: return iPhone
default:
assertionFailure("unrecognised device type: \(UIDevice.current)")
return iPhone
}
#endif
}
}

Copy link
Collaborator

@BPerlakiH BPerlakiH left a comment

Choose a reason for hiding this comment

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

@TheRealAnt, @kelson42 I've updated this PR with a couple of things:

  1. removed an old "hack" around the page layout, which we no longer need
  2. made sure we show the bars:
  • if the device is rotated (landscape/portrait)
  • if the url changes (either by clicking on a link or navigating back and forth by arrows or by dragging from the side of the screen)
  • if we scroll to the very top of the screen. This is a bit of a double defence, as scrolling up should also trigger this, but to be on the safe side, as it turned out it is possible to scroll and navigate at the same time

@kelson42 kelson42 merged commit f35f2c2 into kiwix:main May 24, 2024
1 of 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.

iOS single tap Full screen mode
3 participants