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 toolbar to selected editors with a button to delete it (bug 1863763) #17243

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman
Copy link
Contributor Author

For the moment the aria-label and maybe the tooltip are missing but the strings should be come later.

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.

Leaving a few initial, very quick, observations.

Naming things is hard, but I feel like calling this bin throughout the code is confusing since that mostly describes the content of the icon (which may change at some point).
It would rather seems much more appropriate to describe the functionality, and thus call this e.g. delete (or remove) throughout all the code and in the filename.

web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
web/images/editor-toolbar-bin.svg Outdated Show resolved Hide resolved
src/display/editor/tools.js Outdated Show resolved Hide resolved
src/display/editor/toolbar.js Outdated Show resolved Hide resolved
src/display/editor/toolbar.js Outdated Show resolved Hide resolved
web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
web/annotation_editor_layer_builder.css Outdated Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

/botio-linux preview

@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/5e98ebedcd186e0/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/5e98ebedcd186e0/output.txt

Total script time: 1.42 mins

Published

@Snuffleupagus
Copy link
Collaborator

After playing with this a bit, I've got two observations/questions:

  • If you select multiple editors and then click on the new delete-button only that specific editor is deleted, which while reasonable behaviour in itself may possibly be confusing to users.
    Thus I wonder if we need some way to visually notify the user that only one editor will be removed, or if this is implicitly understood because there's multiple delete-buttons visible in this case (i.e. one for each selected editor)?

    (Compare with the situation where you select multiple editors and then press the Delete key, where all selected editors are being removed.)

  • I've not looked in detail at the design-spec, so it's possible that you've simply implemented what the specification says.

    Would it make sense to give the editToolbar-element itself a little bit of padding (e.g. 1px all around), since in the focus-visible-state the button-outline and the toolbar-border kind of "blends" together now?


    blend

@calixteman
Copy link
Contributor Author

After playing with this a bit, I've got two observations/questions:

* If you select _multiple_ editors and then click on the new delete-button only that specific editor is deleted, which while reasonable behaviour in itself _may_ possibly be confusing to users.
  Thus I wonder if we need some way to visually notify the user that only _one_ editor will be removed, or if this is implicitly understood because there's multiple delete-buttons visible in this case (i.e. one for each selected editor)?
  (Compare with the situation where you select multiple editors and then press the Delete key, where _all selected_ editors are being removed.)

If the user has selected several editors and then click on the delete button I think it makes sense to delete all of them.

* _I've not looked in detail at the design-spec, so it's possible that you've simply implemented what the specification says._
  Would it make sense to give the editToolbar-element itself _a little bit_ of padding (e.g. `1px` all around), since in the `focus-visible`-state the button-outline and the toolbar-border kind of "blends" together now?

Yes, the border line has been recently added to the specs because it helps to have a contrast between the toolbar and a clear background.
The css :has has landed few weeks ago in nightly, so it's time to use it in removing the border when the a button has a focus-outline.

test/integration/freetext_editor_spec.mjs Outdated Show resolved Hide resolved
test/integration/freetext_editor_spec.mjs Outdated Show resolved Hide resolved
web/annotation_editor_layer_builder.css Show resolved Hide resolved
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.

For the moment the aria-label and maybe the tooltip are missing but the strings should be come later.

Should we wait for the l10n-strings to be provided first, or did you intend for this PR to land as-is (with a later follow-up patch)?


r=me, since this seems to work as it should; thank you!

@calixteman
Copy link
Contributor Author

I hope we'll have the strings next week but it's worth having asap in nightly in order to be tested.
So I'll do a follow-up patch once I've them.

@calixteman calixteman merged commit f56215f into mozilla:master Nov 10, 2023
5 checks passed
@adityadees

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants