[Editor] Add a color picker in the toolbar of Ink and Freetext annotations #20070
Conversation
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/01816abba62ab60/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/01816abba62ab60/output.txt Total script time: 0.95 mins Published |
calixteman
force-pushed
the
editor_toolbar_color
branch
2 times, most recently
from
27ae433 to
ab5c1e7
Compare
calixteman
force-pushed
the
editor_toolbar_color
branch
from
ab5c1e7 to
5a03f26
Compare
|
/botio integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.193.163.58:8877/6e495e613178db4/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @calixteman received. Current queue size: 0 Live output at: http://54.241.84.105:8877/9bd421a16698c80/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/9bd421a16698c80/output.txt Total script time: 13.36 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/6e495e613178db4/output.txt Total script time: 27.15 mins
|
| @@ -304,4 +304,61 @@ class ColorPicker { | |||
| } | |||
| } | |||
|
|
|||
| export { ColorPicker }; | |||
| class BasicColorPicker { | |||
There was a problem hiding this comment.
It's not immediately clear to me what the difference is between a basic color picker and the pre-existing regular color picker. It might be good to add a short docstring to both classes to explain the difference to the reader so it's clear which should be used in different scenarios.
| @@ -407,7 +407,7 @@ class HighlightEditor extends AnnotationEditor { | |||
| const colorPicker = (this.#colorPicker = new ColorPicker({ | |||
| editor: this, | |||
| })); | |||
| return [["colorPicker", colorPicker]]; | |||
| return [["highlightColorPicker", colorPicker]]; | |||
There was a problem hiding this comment.
Why is this renamed, because the ink and freetext ones are already both called colorPicker?
| @@ -163,7 +163,7 @@ class EditorToolbar { | |||
|
|
|||
| async addButton(name, tool) { | |||
| switch (name) { | |||
| case "colorPicker": | |||
| case "highlightColorPicker": | |||
There was a problem hiding this comment.
This ties in to the renaming comment above, but this case seems to be identical to the one of colorPicker below. Can we collapse them into one to have a generic color picker addition call for all annotation types, and let the annotation types themselves determine (via toolbarButtons) if they provide the basic or the regular color picker since that seems simpler overall?
| await closePages(pages); | ||
| }); | ||
|
|
||
| it("must check that the stroke color is the one choose from the color picker", async () => { |
|
|
||
| it("must check that the stroke color is the one choose from the color picker", async () => { | ||
| await Promise.all( | ||
| pages.map(async ([_browserName, page]) => { |
There was a problem hiding this comment.
In other places if the parameter is not used we only use an underscore to indicate that; see
, so let's do that there too for consistency:async ([_, page])
| @@ -1294,3 +1294,45 @@ describe("Should switch from an editor and mode to others by double clicking", ( | |||
| ); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("Ink must update its color", () => { | |||
There was a problem hiding this comment.
Could you also add a test like this for freetext annotations so that part of the PR is also covered?
No description provided.