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] Don't show the alt-text button when the alt-text dialog is visible #17032

Merged
merged 1 commit into from
Oct 2, 2023
Merged

[Editor] Don't show the alt-text button when the alt-text dialog is visible #17032

merged 1 commit into from
Oct 2, 2023

Conversation

calixteman
Copy link
Contributor

This way, the button doens't cover the image.

@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/140d5f6cf97d228/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/140d5f6cf97d228/output.txt

Total script time: 1.60 mins

Published

@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/4cb68a2dc4660d2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/4cb68a2dc4660d2/output.txt

Total script time: 1.57 mins

Published

@Snuffleupagus
Copy link
Collaborator

Thinking about the "bigger picture" a little bit here: Would it make sense to instead only show the altText button when the StampEditor is hovered and/or focused?

@Dasha-Andriyenko
Copy link

@Snuffleupagus, while in editing mode, we'd want to show the alt text button even when not hovered/ focused. This is because we'd want to let users know when alt text has been added/ not added even just at a glance. This way, when a user is looking at all the photos, they can easily spot which ones have no alt text vs ones that do.

@Snuffleupagus
Copy link
Collaborator

One thing that I'd "worry" about with these changes is that if something goes wrong somewhere (e.g. if the dialog isn't opened as expected) we might end up with the altText-button being permanently hidden and thus inaccessible.
With that in mind, would it perhaps make sense to instead just forcibly set the small class on the altText-button while the dialog is open? It'd thus be moved "out" from the image, and not obscure it, but would be guaranteed to still be available.

@calixteman
Copy link
Contributor Author

One thing that I'd "worry" about with these changes is that if something goes wrong somewhere (e.g. if the dialog isn't opened as expected) we might end up with the altText-button being permanently hidden and thus inaccessible. With that in mind, would it perhaps make sense to instead just forcibly set the small class on the altText-button while the dialog is open? It'd thus be moved "out" from the image, and not obscure it, but would be guaranteed to still be available.

If something goes wrong after the button is clicked and if one consequence is not having the button anymore, I'd say that it's probably the best way to have a bug report :).
That said if it really happens then why should we care about showing a button which is doing bad stuff ?

@calixteman calixteman force-pushed the alt_text_rm_button branch 2 times, most recently from be31723 to e5dbd73 Compare September 28, 2023 16:24
@calixteman calixteman marked this pull request as ready for review September 28, 2023 16:24
Snuffleupagus
Snuffleupagus approved these changes Oct 2, 2023
View reviewed changes
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, with one more comment; thank you!

src/display/editor/tools.js Outdated Show resolved Hide resolved
@calixteman
[Editor] Don't show the alt-text button when the alt-text dialog is v…
eebd251 
…isible

This way, the button doens't cover the image.
@calixteman
Copy link
Contributor Author

/botio integrationtest

1 similar comment
@calixteman
Copy link
Contributor Author

/botio integrationtest

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

@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/3fd8868fee1dce8/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

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

Total script time: 5.65 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/7d00c97cc823493/output.txt

Total script time: 16.72 mins

  • Integration Tests: FAILED

@calixteman calixteman merged commit 59d94b5 into mozilla:master Oct 2, 2023
3 checks passed
@calixteman calixteman deleted the alt_text_rm_button branch October 2, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
4 participants
@calixteman @moz-tools-bot @Snuffleupagus @Dasha-Andriyenko