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

Media Actions - Better UX #38771

Merged
merged 40 commits into from
Oct 15, 2022
Merged

Conversation

crystalenka
Copy link
Member

@crystalenka crystalenka commented Sep 15, 2022

The current media quick actions, while useful, are confusing as currently displayed:
Screen Shot of media quick action icons

Depending on the media item, the icons are hard to see, and it's not always clear what each icon means (especially with rename). Additionally, when the mouse leaves the relatively small focus area, the buttons disappear and you have to click into it again. This leads to frustration.

Summary of Changes

I have reworked the display of the actions to include text for sighted users and make it more clear what each icon does. Additionally, I changed the icon for "rename" to one that's more accurate (as the arrows before signified changing width or size).

They now display like so:
Screen Shot of revised media quick action icons

It's not a perfect solution - they still disappear when the mouse leaves the focus area - but it should be easier and clearer for people to understand the available functions.

Testing Instructions

As this PR changes build files for JS and CSS, you will have to download the build package from this PR. The patch in the patch tester will NOT work.

Navigate to the media manager. Click on the ellipses icon on an item.

Actual result BEFORE applying this Pull Request

Icon-only actions displayed.
Screen Shot of media quick action icons

Expected result AFTER applying this Pull Request

A more comprehensive menu displays.
Screen Shot of revised media quick action icons

Documentation Changes Required

None that I'm aware of.

Additional Notes

Open to a more elegant solution. Vue.js is not something I'm familiar with so it's possible there's a better way to do this. Regarding accessibility, it should function for keyboard/screenreader users exactly as before.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Sep 15, 2022
@coolcat-creations
Copy link
Contributor

+1000 Very great improvement!

@brianteeman
Copy link
Contributor

Does it need the word "item"? Isnt that a redundant word.

@crystalenka
Copy link
Member Author

crystalenka commented Sep 15, 2022 via email

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Sep 15, 2022
@crystalenka
Copy link
Member Author

Does it need the word "item"? Isnt that a redundant word.

Removed :)

@crystalenka
Copy link
Member Author

Note to self - need to check what happens with super long language strings. I have a feeling it will break if the translation is too long so I need to anticipate that.

@brianteeman
Copy link
Contributor

image

Doesn't seem correct but I can't view the source to check it ;(

@Kostelano
Copy link
Contributor

Removing redundant 'item' from media action language strings

@crystalenka remove also for frontend please.

@crystalenka
Copy link
Member Author

Removing redundant 'item' from media action language strings

@crystalenka remove also for frontend please.

Done, thank you!

@crystalenka crystalenka marked this pull request as draft September 20, 2022 07:38
@crystalenka
Copy link
Member Author

Converting to draft, as it's not even close to accessible for keyboard users.

@crystalenka crystalenka marked this pull request as ready for review September 20, 2022 07:49
@crystalenka
Copy link
Member Author

This is ready for review and testing.

There are KNOWN ISSUES for accessibility in the media manager (keyboard access, focus styles, etc) however fixing those would require considerable changes to the markup of the media manager and are outside the scope of this PR. This PR does not introduce additional barriers to access and hopefully mitigates some prior a11y issues regarding understanding of the actions, etc.

@chmst can we discuss in JAT the best way to approach the accessibility here? I am sure it will require minor breaking changes given how everything is currently generated with vue.js

@coolcat-creations
Copy link
Contributor

There is a z-index issue, sorry!
grafik

@HLeithner
Copy link
Member

@HLeithner can you please explain your comment about it changing the meaning of the string? The string changed but it is basically the same thing. "Open item actions" implies "...to manage this item". "Manage item: (filename)" is more direct but at least in English, the same thing. What is the benefit of adding a new string when the old one won't be used anywhere else?

Since @brianteeman can only say "no it is not" and doesn't explain why it's not, I try to explain why it is.

When the count of wildcards don't match you generate a warning in the translation function.

@crystalenka
Copy link
Member Author

I see, thanks for explaining. But in order to add a new string instead the old string will be removed, so it will just be a broken language string if it's not added. (Right?) Is that better than an old string and a PHP warning?

As I'm not a translator, I want to understand the best practices here for future reference.

@HLeithner
Copy link
Member

I see, thanks for explaining. But in order to add a new string instead the old string will be removed, so it will just be a broken language string if it's not added. (Right?) Is that better than an old string and a PHP warning?

As I'm not a translator, I want to understand the best practices here for future reference.

We simple don't remove language strings in minor/patch levels, or change the meaning of the string.

https://developer.joomla.org/news/586-joomla-development-strategy.html#6.1.6%20Language%20keys:~:text=them%20is%20not.-,6.1.6%20Language%20keys,-Changing%20or%20deleting

6.1.6 Language keys
Changing or deleting a language key is considered a backwards compatibility break. Adding new ones is not. Substantially changing the meaning associated with a language key is a compatibility break. Rephrasing something for a more accurate description or proper en-GB grammar is not.

I know some people don't like this policy but since they don't push an update for the policy forward it is still valid.

@crystalenka
Copy link
Member Author

I would argue that it's rephrasing for a more accurate description in this case. "Open item actions" is unclear/borderline useless in the context it's used (as title or aria label for screen readers).

@HLeithner
Copy link
Member

you can do that but then you can't add %s to it.

@crystalenka
Copy link
Member Author

Well, that's why it's borderline useless for screenreaders—there's no accessible indication of which item you're managing. :)

But if the addition of the string variable (I dunno the proper term for it) is a breaking change I guess I will add a new string so that this can be merged. I just hope that it won't make it less accessible for users in using it in other languages if it doesn't get translated in time.

@HLeithner
Copy link
Member

But if the addition of the string variable (I dunno the proper term for it) is a breaking change I guess I will add a new string so that this can be merged. I just hope that it won't make it less accessible for users in using it in other languages if it doesn't get translated in time.

if the language translators don't update the language then the language is dead and we should find new translators for the language or remove it at some point. So don't worry about new language strings. Only add a notice to the old string that it is deprecated something like:
// Deprecated will be removed with 5.0

thanks

@brianteeman
Copy link
Contributor

When the count of wildcards don't match you generate a warning in the translation function.

That might be true but its not applicable here and no warning is issued.

@brianteeman
Copy link
Contributor

The only time I can see that the chnge here could ever produce an error would be if you used a new language file with an old version of joomla but surely we dont support that anyway. Using an old language file with a new joomla version does not produce an error in this specific case.

@crystalenka
Copy link
Member Author

@HLeithner I've added a new language string; not sure how to mark the old one as deprecated since it shows up both in the .ini files and in the default_texts.php file that passes the language strings to vue.

@crystalenka
Copy link
Member Author

crystalenka commented Oct 12, 2022

@HLeithner I've added deprecation notices in the .ini files.

The old language string is also referenced here:

Text::script('COM_MEDIA_OPEN_ITEM_ACTIONS', true);

I'm not sure how to deprecate that. If it's even necessary? I would really really really love to be done with this pull request, 128 comments later... 🤪

@HLeithner
Copy link
Member

@HLeithner I've added deprecation notices in the .ini files.

The old language string is also referenced here:

Text::script('COM_MEDIA_OPEN_ITEM_ACTIONS', true);

I'm not sure how to deprecate that. If it's even necessary? I would really really really love to be done with this pull request, 128 comments later... 🤪

thanks, no it's not needed to deprecated in the defaults_texts.php, but you could write an PR against 5.0 which removes both ;-)

@crystalenka
Copy link
Member Author

@HLeithner I've added deprecation notices in the .ini files.
The old language string is also referenced here:

Text::script('COM_MEDIA_OPEN_ITEM_ACTIONS', true);

I'm not sure how to deprecate that. If it's even necessary? I would really really really love to be done with this pull request, 128 comments later... 🤪

thanks, no it's not needed to deprecated in the defaults_texts.php, but you could write an PR against 5.0 which removes both ;-)

Ahhhhhh!!! And how many comments will that one get? 🙈 I'll wait, in case there are other changes to media manager in the meantime...

@HLeithner
Copy link
Member

Ahhhhhh!!! And how many comments will that one get? 🙈 I'll wait, in case there are other changes to media manager in the meantime...

You are right don't write a PR because it has to be keep till 6.0, maybe moved into the legacy plugin

@crystalenka
Copy link
Member Author

Are there any other changes you'd like me to make @HLeithner? I still see the change requested flag. :)

@roland-d roland-d merged commit 8f684f1 into joomla:4.2-dev Oct 15, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 15, 2022
@roland-d
Copy link
Contributor

Thank you.

@roland-d roland-d added this to the Joomla! 4.2.4 milestone Oct 15, 2022
@crystalenka crystalenka deleted the media-actions-ux branch October 15, 2022 17:11
heelc29 added a commit to heelc29/joomla that referenced this pull request Oct 24, 2022
Kostelano added a commit to JPathRu/localisation that referenced this pull request Oct 25, 2022
joomla/joomla-cms#37385 - (только для pl-PL)
joomla/joomla-cms#38825 +
joomla/joomla-cms#38877 - (только для других языков)
joomla/joomla-cms#38867 +
joomla/joomla-cms#37017 +
joomla/joomla-cms#38771 +
joomla/joomla-cms#38761 +
joomla/joomla-cms#39072 - (только для en-GB)
+ исправление различных формулировок
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.