WIP: FXL/Divina improvements - #198
Conversation
2d76a07 to
b65e147
Compare
…ments Conflict resolutions: - Manifest.ts: combine develop's optional `links` property and subcollections fix (#225) with this branch's empty-links coalescing and omit-empty-links serialization; strict-safe serialize condition - css.ts: keep both isTypedOMSupported (branch) and styleChangeAffectsLayout (develop) - package.json (x3): take develop's released versions - FXLFramePoolManager.ts: underscore the unused `layout` param (pre-existing TS6133 at branch tip) pending scrolled-layout WIP Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| simg.decoding = "async"; | ||
| simg.src = burl || ""; | ||
| simg.alt = link.title || ""; | ||
| await simg.decode(); // Reduce repaints |
There was a problem hiding this comment.
What happens in case of an EncodingError? There is no guard here so I’m wondering if it could turn into a hard failure for the whole page build or if that is fine.
|
|
||
| public async destroy() { | ||
| // Flag synchronously so an in-flight load() bails before attaching frames. | ||
| this._destroyed = true; |
There was a problem hiding this comment.
Does WebPubNavigator need this too?
| private _isNavigating: boolean = false; | ||
| private _isStalled: boolean = false; | ||
| /** Set by destroy(); stops the deferred initial load from touching a dead instance. */ | ||
| private _destroyed: boolean = false; |
There was a problem hiding this comment.
Should this guard mediaElementSetup in addition to startInitialLoad? I am thinking about an edge case of a synchronous destroy() running setup against an orphaned element.
| quality?: DivinaQuality | null, | ||
| scrolled?: boolean | null, | ||
| spreads?: boolean | null, | ||
| stripWidth?: number | null |
There was a problem hiding this comment.
I guess that eventually we want to consolidate this with an additional fit preference/setting e.g. fit/cover/contain, etc. – I don’t remember the values exactly. Note a fit preference also applies to FXL so it is not specifically Divina.
| clear(): void { | ||
| this.preferences = new DivinaPreferences({}); | ||
| } |
There was a problem hiding this comment.
Note I recently corrected that, has all values have to be nullified – otherwise they are undefined on merge and it is a no-op
| keyboardPeripherals?: IKeyboardPeripheralsConfig; | ||
| } | ||
|
|
||
| export interface DivinaNavigatorListeners { |
There was a problem hiding this comment.
As a reminder, we need to add timelineItemChanged to that list – and in publication as well.
| export enum DivinaQuality { | ||
| auto = "auto", // Match the display resolution | ||
| low = "low", // Smallest available variant | ||
| high = "high", // One step above the display resolution | ||
| max = "max", // Largest available variant | ||
| } |
There was a problem hiding this comment.
I guess if feasible it could be useful to have an accompanying helper in shared deriving what’s available from manifest so that consumers can pick the values they provide, chose not to display the setting if there is no variant, etc.
Of course that means having opinionated heuristics for corner cases.
| private isDarkColor(color: string): boolean | null { | ||
| const doc = this.container.ownerDocument; | ||
| const probe = doc.createElement("div"); | ||
| probe.style.display = "none"; | ||
| probe.style.color = color; | ||
| doc.body.appendChild(probe); | ||
| const rgb = (doc.defaultView || window).getComputedStyle(probe).color; | ||
| probe.remove(); | ||
| const m = rgb.match(/rgba?\(\s*(\d+)[,\s]+(\d+)[,\s]+(\d+)/); | ||
| if(!m) return null; | ||
| const luminance = (0.2126 * +m[1] + 0.7152 * +m[2] + 0.0722 * +m[3]) / 255; | ||
| return luminance < 0.45; | ||
| } |
There was a problem hiding this comment.
I guess that if it does not differ too much – that could use the color helpers from the helpers package as we are checking dark/light in Decorator module already.
Of course the color helper itself could be improved and/or updated for versatility.
|
Some first inputs, more than a proper review as there is some catching up to do and the PR is still in draft. Overall, I do not see anything problematic. On the opposite, it resolves some issues. Preferences API is implemented as expected, and the The biggest visible gap is the absence of Timeline API but this is to be expected as well, as it is fairly new in the develop branch. Other than that, looks very positive to me, especially after testing the implementation in a reading app. |
4 participants
No description provided.