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.

Sign up for GitHub

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

Jump to bottom

[Editor] Add a toggle button to show/hide all the highlights (bug 1867740) #17778

Merged
merged 1 commit into from
Mar 7, 2024
Merged

[Editor] Add a toggle button to show/hide all the highlights (bug 1867740) #17778

merged 1 commit into from
Mar 7, 2024

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman calixteman requested a review from Snuffleupagus March 6, 2024 21:39
@calixteman calixteman force-pushed the highlight_show_all branch 2 times, most recently from 749c47d to dc1d34c Compare March 7, 2024 09:32
@calixteman calixteman requested a review from a team as a code owner March 7, 2024 09:32
Snuffleupagus
Snuffleupagus reviewed Mar 7, 2024
View reviewed changes
src/display/editor/tools.js Outdated Show resolved Hide resolved
src/display/editor/tools.js Show resolved Hide resolved
web/toggle_button.css Outdated Show resolved Hide resolved
web/toggle_button.css Outdated Show resolved Hide resolved
web/toggle_button.css Show resolved Hide resolved
web/annotation_editor_params.js Show resolved Hide resolved
@calixteman calixteman force-pushed the highlight_show_all branch from dc1d34c to 39aeea3 Compare March 7, 2024 12:17
@Snuffleupagus
Copy link
Collaborator

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/24af36e57e29bf0/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/24af36e57e29bf0/output.txt

Total script time: 1.29 mins

Published

@Snuffleupagus
Copy link
Collaborator

One question, based on testing the preview, however this may not be something that we want to implement:
Do we want to enable "Show all" unconditionally, when Highlighting mode is enabled (e.g. via the toolbar)?

@calixteman
Copy link
Contributor Author

One question, based on testing the preview, however this may not be something that we want to implement: Do we want to enable "Show all" unconditionally, when Highlighting mode is enabled (e.g. via the toolbar)?

Good question and another came in my mind: what should we do when showAll is off and the user leaves the Highlighting mode ?
Let ask to @caseyr28 what he thinks about that.

@caseyr28
Copy link

caseyr28 commented Mar 7, 2024

One question, based on testing the preview, however this may not be something that we want to implement: Do we want to enable "Show all" unconditionally, when Highlighting mode is enabled (e.g. via the toolbar)?

I think in the future having a better affordance for the state of tools and annotations makes sense. This should be a part of the longer term UX debt improvements as there are many ways to solve this, and I would want to ensure scalability.

Good question and another came in my mind: what should we do when showAll is off and the user leaves the Highlighting mode ?

I think the flow now works; a user that toggles visibility off and disables the tool can enable the tool and either toggle visible, or begin highlighting and have visibility=true. I think this should be the same for when a user creates a highlight via selecting text/in-content. One caveat is that the toggle button may not be enough affordance to indicate state, which IMO relays back to the first question.

@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/3c7463cf2a4bebd/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/56fb383e162c65b/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/3c7463cf2a4bebd/output.txt

Total script time: 6.30 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/56fb383e162c65b/output.txt

Total script time: 15.40 mins

  • Integration Tests: Passed

@calixteman calixteman merged commit 4060189 into mozilla:master Mar 7, 2024
10 checks passed
@calixteman calixteman deleted the highlight_show_all branch March 7, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flodolo flodolo flodolo approved these changes

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@calixteman @Snuffleupagus @moz-tools-bot @caseyr28 @flodolo