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

[Editor] Add a color picker with predefined colors for highlighting text (bug 1866434) #17359

Merged

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented Nov 30, 2023

The doorhanger for highlighting has a basic color picker composed of 5 predefined colors
to set the default color to use.
These colors can be changed thanks to a preference for now but it's something which could
be changed in the Firefox settings in the future.
Each highlight has in its own toolbar a color picker to just change its color.
The different color pickers are so similar (modulo few differences in their styles) that
this patch introduces a new class ColorPicker which provides a color picker component
which could be reused in future editors.
All in all, a large part of this patch is dedicated to color picker itself and its style
and the rest is almost a matter of wiring the component.

@calixteman
Copy link
Contributor Author

/botio-linux preview

@mozilla mozilla deleted a comment from moz-tools-bot Nov 30, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Nov 30, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Nov 30, 2023
@mozilla mozilla deleted a comment from moz-tools-bot Nov 30, 2023
@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/644682722003ebf/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/644682722003ebf/output.txt

Total script time: 1.42 mins

Published

@calixteman calixteman force-pushed the editor_highlight_color_picker branch 3 times, most recently from d2fff1b to 9bc82b5 Compare November 30, 2023 16:28
l10n/en-US/viewer.ftl Show resolved Hide resolved
@Snuffleupagus
Copy link
Collaborator

Something that would really help (and reduce the time it takes me to review this), given the size/scope of these changes, is a high-level overview of what's being changed and why here.
Can you pretty please write a paragraph, or even two, explaining these changes (maybe put it in the commit message as well)?

Skimming this, there's a few things that I'm not totally sure of:

  • Do we actually need to "generate" the viewer-UI highlightToolbar?
    Why can't we keep the HTML code in the viewer, to (slightly) lessen the amount of changes here?

  • Why are the colors specified in a preference, since that seems to add overall complexity?
    Furthermore, is this intended to be temporary during development/testing (there's no comment about this)?
    Given the new localization-strings, for the colour-names, it doesn't seem (to me) that e.g. the user changing this preference would make a lot of sense?

@calixteman calixteman force-pushed the editor_highlight_color_picker branch 2 times, most recently from 25d1b77 to 3a6ad34 Compare December 1, 2023 15:54
@Snuffleupagus
Copy link
Collaborator

I'm slowly starting to review this, but before looking in detail at the code I've got some observations based on playing with the preview in #17359 (comment) above.

  • Nit: There's a typo in the commit message, hightlighting text -> highlighting text.

  • Looking at the new editorHighlight viewer-toolbar, the padding feels ever so slightly "off" compared to the existing editor-toolbars:

    • The padding-top value seems to be a little bit too large, resulting in what feels like slightly too much empty space at the top of the new toolbar.
    • When the left-most color is hovered/selected the focus-ring extends "beyond" the left edge of the "Highlight"-label,
      which again seems a little weird.

    toolbar

  • The new editorHighlight viewer-toolbar probably doesn't handle RTL locales correctly, since the "Highlight"-label is still left-aligned.

    rtl

  • The button order on the new "inline" editorHighlight-toolbar seems inconsistent, when compared with the editorStamp-toolbar; note the red outlines in the screen-shot below.
    In my opinion, having the Delete-button "move around" depending on the editor-type doesn't seem great. First of all it just feels inconsistent, and secondly it might lead to users accidentally clicking the Delete-button when moving from one editor-type to another.
    For any button(s) that all editors implement, I really think that it should have the same relative position in all editors.

    inline

@caseyr28
Copy link

caseyr28 commented Dec 4, 2023

Hi @Snuffleupagus - UX chiming in here. Thanks for the review and feedback.

  1. editorHighlight viewer-toolbar padding; Agreed, we will make consistent with the other toolbars.
  2. Left-most icon alignment; This was designed that intentionally, as otherwise the unselected state would have an indent that would appear off, in my opinion. I am taking some inspiration from the typographic principle of "Hanging punctuation" - obviously these are not all characters, but optically the color-swatch itself should be the main-aligned element.
  3. The button order on the new "inline" editorHighlight-toolbar; Agreed, thanks for catching that. To be updated.
  4. handle RTL locales; @calixteman let me know if we need to add specs for this.

Thanks

@Snuffleupagus Snuffleupagus changed the title [Editor] Add a color picker with predefined colors for hightlighting text (bug 1866434) [Editor] Add a color picker with predefined colors for highlighting text (bug 1866434) Dec 5, 2023
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

One thing that I noticed, which might be on purpose but feels a little bit inconsistent, is that after you'd added a highlightEditor its color can only be changed with the editToolbar-dropdown.
However, trying to change the color from the viewer-toolbar does nothing for a currently selected highlightEditor.

web/app_options.js Outdated Show resolved Hide resolved
src/display/editor/color_picker.js Outdated Show resolved Hide resolved
src/display/editor/color_picker.js Show resolved Hide resolved
src/display/editor/color_picker.js Outdated Show resolved Hide resolved
src/shared/util.js Outdated Show resolved Hide resolved
web/images/editor-toolbar-colorpicker-arrow.svg Outdated Show resolved Hide resolved
web/plop.html Outdated Show resolved Hide resolved
web/annotation_editor_layer_builder.css Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

One thing that I noticed, which might be on purpose but feels a little bit inconsistent, is that after you'd added a highlightEditor its color can only be changed with the editToolbar-dropdown. However, trying to change the color from the viewer-toolbar does nothing for a currently selected highlightEditor.

Yep it's on purpose: the color in the main toolbar is the default color to use.

@calixteman calixteman force-pushed the editor_highlight_color_picker branch 2 times, most recently from 94f5084 to e8ff0c1 Compare December 5, 2023 18:52
…ext (bug 1866434)

The doorhanger for highlighting has a basic color picker composed of 5 predefined colors
to set the default color to use.
These colors can be changed thanks to a preference for now but it's something which could
be changed in the Firefox settings in the future.
Each highlight has in its own toolbar a color picker to just change its color.
The different color pickers are so similar (modulo few differences in their styles) that
this patch introduces a new class ColorPicker which provides a color picker component
which could be reused in future editors.
All in all, a large part of this patch is dedicated to color picker itself and its style
and the rest is almost a matter of wiring the component.
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, thank you!

@calixteman
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/b1bb3b7f2d55957/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/90fc1d49b325552/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/b1bb3b7f2d55957/output.txt

Total script time: 5.77 mins

  • Integration Tests: FAILED

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/90fc1d49b325552/output.txt

Total script time: 13.93 mins

  • Integration Tests: FAILED

@calixteman calixteman merged commit 8702e1b into mozilla:master Dec 6, 2023
10 checks passed
@calixteman calixteman deleted the editor_highlight_color_picker branch December 6, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants