Combine book-navigator and ia-bookreader web components#1492
Combine book-navigator and ia-bookreader web components#1492cdrini merged 5 commits intointernetarchive:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1492 +/- ##
==========================================
+ Coverage 69.68% 69.70% +0.01%
==========================================
Files 64 65 +1
Lines 5373 5373
Branches 1179 1182 +3
==========================================
+ Hits 3744 3745 +1
Misses 1594 1594
+ Partials 35 34 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jbuckner
left a comment
There was a problem hiding this comment.
One question about consolidation, but overall looks good!
I only did a quick scan of the ia-bookreader.js updates since it's pretty large and I assume most of it is just consolidating book-navigator.js
There was a problem hiding this comment.
Pull request overview
This pull request combines the book-navigator and ia-bookreader web components to reduce DOM layers and simplify the component hierarchy. The book-navigator component has been completely removed, with all its functionality absorbed into ia-bookreader.
Changes:
- Merged
book-navigatorcomponent intoia-bookreader, eliminating a layer of DOM nesting - Changed property name from
itemMDtoitemfor consistency - Converted event-based communication to direct property updates within the consolidated component
- Updated internal property names (e.g.,
brWidth→_brWidth) to follow private naming convention
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BookNavigator/book-navigator.js | Entire file deleted - functionality moved to ia-bookreader.js |
| src/ia-bookreader/ia-bookreader.js | Major refactor: absorbed all book-navigator functionality, converted event emissions to direct property updates |
| tests/jest/ia-bookreader/ia-bookreader.test.js | Updated tests for consolidated component, renamed properties, changed event assertions to direct property/method checks |
| src/BookReader.js | Updated TypeScript type comment to reference IaBookReader instead of BookNavigator |
| src/BookNavigator/visual-adjustments/visual-adjustments-provider.js | Simplified constructor parameter destructuring |
| src/BookNavigator/viewable-files.js | Added JSDoc documentation for constructor parameters |
Comments suppressed due to low confidence (2)
tests/jest/ia-bookreader/ia-bookreader.test.js:280
- The test name includes "
@updateSideMenu" with an @ symbol, but this appears to be leftover from when this was an event. SinceupdateSideMenuis now a direct method call (not an event), remove the @ symbol for clarity.
tests/jest/ia-bookreader/ia-bookreader.test.js:31 - The baseHost property is set to
https://foo.archive.org(with protocol), but the default value in the constructor isarchive.org(without protocol). TheitemImagegetter prepends "https://" which would result in "https://https://foo.archive.org/..." if baseHost includes the protocol. Consider setting baseHost to justfoo.archive.orgwithout the protocol to match the expected usage pattern and default value.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4225256 to
eeafa86
Compare
While working on hideable chrome, I will want to later also make the book actions bar be hideable. The positioning of the toolbars is a bit difficult (especially when going fullscreen), and the two layers of web components here made this more difficult. Since we know we long-term want one main wrapping component,
ia-bookreaderwithia-item-navigatoras its shell, combiningbook-navigatorintoia-bookreaderremoves a decent amount of layers of DOM and helps us get closer to that vision!