Migrate annotation changes - #1554
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1554 +/- ##
==========================================
- Coverage 63.81% 59.63% -4.18%
==========================================
Files 69 72 +3
Lines 6256 6826 +570
Branches 1391 1518 +127
==========================================
+ Hits 3992 4071 +79
- Misses 2225 2710 +485
- Partials 39 45 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cc2e50c to
274ddda
Compare
cdrini
left a comment
cdrini
left a comment
There was a problem hiding this comment.
Got through the first half ; will finish the rest later today!
b205c70 to
1ac3b87
Compare
467afe4 to
2ed063b
Compare
3f9114e to
ae15b93
Compare
Fix renamed parameter within markRange function
2855f69 to
a738684
Compare
| const BR_HIGHLIGHTS_LOCAL_STORAGE_KEY = "BRhighlightStorage"; | ||
|
|
||
| @customElement('br-annotation-modal') | ||
|
|
| } | ||
|
|
||
| renderColorOptions() { | ||
| const colors = { |
There was a problem hiding this comment.
This appears to be duplicating the static constants on this class also below where have another similar object. These might be able to be DRY'd up somehow so that we're not duplicated the color names / hex values.
| placement="bottom-start" | ||
| .anchor=${this.anchorEl} | ||
| .open=${live(this.open)} | ||
| ._position=${this.positionObj} |
There was a problem hiding this comment.
This seems like something that shouldn't be private ; we can like make _position just position.
| } | ||
|
|
||
| updateTextArea(text) { | ||
| const inputEle = document.querySelector("#annotateTextArea"); |
There was a problem hiding this comment.
| const inputEle = document.querySelector("#annotateTextArea"); | |
| const inputEle = this.querySelector("#annotateTextArea"); |
| @@ -0,0 +1,57 @@ | |||
| /** | |||
There was a problem hiding this comment.
Add a link to the OL file so we can potentially in theory at some point sync/updated.
| if (!this.activeHighlightNodes) { // highlight selection if not already done | ||
| this.handleHighlightSave(); | ||
| } | ||
| this.br.plugins.textSelection?.textSelectionManager.showAnnotationModal(this.activeHighlightNodes, anchorEl); |
There was a problem hiding this comment.
I think we need to create the plugin annotations class matching the other plugins, so that we can attach the annotation-specific methods/data/logic to that alone.
That also keeps/fulfills the promise of BR plugins being optional. Letting people load the plugin files only if they support/want to support the associated functionality.
| this.br.plugins.textSelection?.textSelectionManager.showAnnotationModal(this.activeHighlightNodes, anchorEl); | |
| this.br.plugins.annotations.showAnnotationModal(this.activeHighlightNodes, anchorEl); |
| * @param {HTMLElement} [anchorEl] Element to anchor the annotation popover | ||
| * to, e.g. the br-menu-option button that was clicked | ||
| */ | ||
| showAnnotationModal(nodes, anchorEl) { |
There was a problem hiding this comment.
These sorts of methods would make sense to move to a dedicated AnnotationPlugin
| * Saves all the highlights in an array to localStorage | ||
| * @param {BookReaderSavedHighlight[]} highlights | ||
| */ | ||
| function saveToLocalStorage(highlights) { |
There was a problem hiding this comment.
Should these still exist? Or should these now be done by the StorageService?
I think we want to treat highlights and annotations as the same thing. They're both "Annotations" ; one just has a comment attached to it, and one doesn't.
| endTextNode.parentElement.classList.contains("BRhighlight")) { | ||
| return; | ||
| const existingMarks = []; | ||
| const walker = document.createTreeWalker( |
There was a problem hiding this comment.
Is there anyway we can re-use some of the helper methods we have? Not sure.
|
UI feedback:
|
2 participants
Migrated annotation code to work with newly improved main branch