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

Annotorious upgrade, delete key fix (#1221), polygon tool (#1311) #1456

Merged
merged 7 commits into from
Sep 11, 2023

Conversation

blms
Copy link
Contributor

@blms blms commented Sep 5, 2023

In this PR

  • Fix a small issue with the enlarge button trying to initialize in the editor
  • Upgrade annotorious, annotorious-toolbar, and annotorious-openseadragon to latest versions

Per #1221:

  • Use the new disableDeleteKey option in Annotorious to fix a bug with editing annotation labels

Per #1311:

  • Add a toolbar with the rectangle and polygon tools from Annotorious to allow creation of polygon image annotations within the editor

@blms blms requested a review from rlskoeser September 5, 2023 17:48
@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #1456 (bcba810) into develop (b9d3aa6) will increase coverage by 0.08%.
Report is 5 commits behind head on develop.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1456      +/-   ##
===========================================
+ Coverage    98.69%   98.77%   +0.08%     
===========================================
  Files          205      205              
  Lines        11399    12272     +873     
===========================================
+ Hits         11250    12122     +872     
- Misses         149      150       +1     

<button class="primary popout-close-button" type="button" aria-label="Close"><i class="ph-x"></i></button>
<button class="popout-button" type="button">
<svg>
<use xlink:href="{% static 'img/ui/desktop/all/pop-out.svg' %}#pop-out-icon" />
</svg>
<span class="sr-only">Pop out</span>
</button>
<fieldset class="toolbar">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these added to the geniza template instead of to tahqiq?

I don't object necessarily, just want to understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't thought to do that! Was conceptualizing it as a Geniza feature so put it in Geniza, but I think it makes more sense to pass the toolbar element (in this case the fieldset) as a param to tahqiq, and instantiate the button elements there.

{# rectangle tool #}
<label class="rect-tool active-tool">
<input type="radio" checked />
<span class="sr-only">Rectangle tool</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how meaningful "rectangle tool" is; how about rephrasing to describe better what these do?

Suggested change
<span class="sr-only">Rectangle tool</span>
<span class="sr-only">Select a rectangle</span>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will put this suggestion into the tahqiq update, thanks!

@blms blms requested a review from rlskoeser September 11, 2023 18:57
const anno = Annotorious(viewer, { disableDeleteKey: true });

// get toolbar element
const toolbarContainer = this.element.querySelector(".tahqiq-toolbar");
Copy link
Contributor

Choose a reason for hiding this comment

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

do you still need to bind this here? what does the annotation controller need to do with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The annotation controller needs to pass it to tahqiq instantiation, but doesn't need to define a variable for it so I've simplified.

@blms blms requested a review from rlskoeser September 11, 2023 19:39
@blms blms merged commit 0a9299e into develop Sep 11, 2023
10 checks passed
@blms blms deleted the bugfix/1221-delete branch September 11, 2023 20:07
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.

2 participants