User tests: Successful: Unsuccessful:
The current media quick actions, while useful, are confusing as currently displayed:
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.
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).
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.
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.
A more comprehensive menu displays.
None that I'm aware of.
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_media NPM Change Repository |
Labels |
Added:
NPM Resource Changed
?
|
Does it need the word "item"? Isnt that a redundant word.
It’s very much redundant; that’s what the language strings were to begin with. Was considering removing it alsoOn Sep 15, 2022, at 4:35 PM, Brian Teeman @.***> wrote:
Does it need the word "item"? Isnt that a redundant word.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: @.***>
Does it need the word "item"? Isnt that a redundant word.
Removed :)
Category | Administration com_media NPM Change Repository | ⇒ | Administration com_media NPM Change Language & Strings Repository |
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.
Removing redundant 'item' from media action language strings
@crystalenka remove also for frontend please.
Labels |
Added:
Language Change
|
Removing redundant 'item' from media action language strings
@crystalenka remove also for frontend please.
Done, thank you!
Converting to draft, as it's not even close to accessible for keyboard users.
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
I have tested this item
Thank you the new menu makes much more sense. Tested successfully.
I have tested this item
Nice improvements
Converting this back to draft. Mobile is a mess. I'm frustrated.
Leave mobile to another pr
@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
Thanks so much, this is a nice improvement .JAT noted a11y issues since the very begining, some have been resolved, some still are open. Implementing is a challenge, as vue.js is requried, as you say.
It would be so great if the name of the item could be added to the task instead of "item" - but need help in vue.js. @angieradtke suggests "media item" instead of "item" which can be everything
Can you not just hide the item text on small screens and then have another look on mobile in a follow up pr?
It would be really helpful if JAT created issues for any accessibility issues so that developers are able to work on them. If they're not on github no one knows
It would be so great if the name of the item could be added to the task instead of "item" - but need help in vue.js. @angieradtke suggests "media item" instead of "item" which can be everything
in what whay does adding the word item or the words media item add to the meaning of this option?
Can you not just hide the item text on small screens and then have another look on mobile in a follow up pr?
The mobile functionality is unrelated to the language string discussion. It doesn't work.
Leave mobile to another pr
It's broken enough that I don't feel comfortable leaving it as-is.
Labels |
Added:
a11y
|
I don't know HOW I managed it but I figured out the Vue enough to get the item name and put it in an sr-only span as a description of the button.
Hard to debug, so the rendered HTML for each button is:
<button type="button" class="action-rename" aria-describedby="rename-desc">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
<span class="sr-only" id="rename-desc">Rename item: joomla_black.png</span>
</button>
I imagine this will work better than generic 'rename item' etc. :)
Now to get it working better in mobile so I can open this again for testing...
Doesn't it now become
Button, rename joomla.png, rename
?
Yes I think its now: Rename Rename item: joomla_black.png
Tested with Narrator
Rename item: joomla_black.png
Tested with NVDA
Rename item: joomla_black.png
button Rename item: joomla_black.png
And that of course is not taking into account that there are six buttons which will all be announced with the potentially unpronouncable filename.
This is trying to fix a problem that does not exist - which I am sure @angieradtke will realise when she gets back from holiday and is in front of a computer
Category | Administration com_media NPM Change Repository Language & Strings | ⇒ | Administration com_media NPM Change JavaScript Language & Strings Repository |
Tested with Narrator
Rename item: joomla_black.png
Tested with NVDA
Rename item: joomla_black.png
button Rename item: joomla_black.pngAnd that of course is not taking into account that there are six buttons which will all be announced with the potentially unpronouncable filename.
This is trying to fix a problem that does not exist - which I am sure @angieradtke will realise when she gets back from holiday and is in front of a computer
I was hoping that by using aria-describedby it would be an optional readout instead of reading out everything. Looking into it...
Perhaps aria-details would be better? I don't believe that is read automatically.
Perhaps aria-details would be better? I don't believe that is read automatically.
even better than that it is rarely read at all as most screen readers do not support it
I am going to leave this PR as-is; it can always be better, but this is still a significant improvement to the current pattern.
In a future PR I will rework the html/js here so that it's a fully accessible disclosure widget; as it's structured now, anything further I could do would just be covering up the symptom instead of addressing the root of the problem.
But for now, it's better.
Ready for re-testing and review.
I have not tested this item.
I have tested this item
sorry pressedwrong radio on my phone.
This is better than it was yesterday
I have tested this item
Tested on desktop (Safari, Edge) and mobile (Safari on iPhone and iPad). It's not perfect (nor can it be without trashing it and starting afresh) but it's now far more usable than the Media Manager before this PR was applied on both desktop and mobile.
On mobile I had to increase the preview size — which I do anyway as I don't have Superman's telephoto vision — and then it's working fine for me. It's far, FAR better than it was before. I'll take it! I mark this successful because, in the words of Steve Jobs, “Real artists ship”.
@richard67 I updated the latest changes from the branch but it previously had 2 successful tests (whoops), no real changes after that. Is there anything else that we need to do here before it can be reviewed or merged?
RTC
Status | Pending | ⇒ | Ready to Commit |
RTC
@crystalenka Human test results get reset in the test counter of the issue tracker after any commit. Therefore I recommend to update the branch only if necessary when a PR has some tests because searching for PRs with 2 tests will work in the issue tracker, which simplifies it a lot for me.
I have restored the test results now and set RTC.
Good to know, thank you!
suggest:
<button type="button" class="action-rename" aria-describedby="rename-desc">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
<span class="sr-only" id="rename-desc">item: joomla_black.png</span>
</button>
This will be announced by screen readers as "Rename, item: joomla_black.png"
The use of aria-details
in this context is not appropriate as the additional information clearly falls into the category of an accessible description. aria-details
is also not as robustly implemented/supported as aria-describedby
The
aria-details
property is for referencing elements that provide more detailed information than would normally be provided via aria-describedby.
@stevefaulkner Many thanks for your advice, it is so helpful
2 last questions for better understanding, futher implementation and our documentation:
aria-describedby -> I read it is possible to reference an element even if that element is hidden.
Does it mean that it can be used for elements that are set to display:none als well or only for elments using visibility:hidden ?
Or for elements set out of the viewport?
In our case the <span class="sr-only" id="rename-desc">item: joomla_black.png</span>
is set out of the viewport.
<button type="button" class="action-rename" >
Is the result without using the attribute aria-describedby not the same, because the span is an part of the button contents?
I ask because if it is like that - we can save lots of code.
Thank you
@stevefaulkner Many thanks for your advice, it is so helpful
2 last questions for better understanding, further implementation and our documentation:
aria-describedby -> I read it is possible to reference an element even if that element is hidden. Does it mean that it can be used for elements that are set to display:none als well or only for elments using visibility:hidden ? Or for elements set out of the viewport? In our case the
<span class="sr-only" id="rename-desc">item: joomla_black.png</span>
is set out of the viewport.
Yes, in fact for the pattern i described to work as intended the description must be hidden usingdisplay:none
orvisibility:hidden
otherwise the desc text will be included in the accessible name as well.
This may be helpful: Hiding Content Has No Effect on Accessible Name or Description Calculation
<button type="button" class="action-rename" >
Is the result without using the attribute aria-describedby not the same, because the span is an part of the button contents?I ask because if it is like that - we can save lots of code.
in this case the accessible name for the button is "Rename" if the sr-only text is hidden using CSS
<button type="button" class="action-rename">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
<span class="sr-only" id="rename-desc">item: joomla_black.png</span>
</button>
some alternatives, with less code:
<button type="button" class="action-rename" aria-description="item: joomla_black.png">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
</button>
<button type="button" class="action-rename" aria-label="Rename item: joomla_black.png">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
</button>
The code in this pr produces a list made of buttons
<div class="media-browser-actions-list">
<ul>
<li aria-hidden="true" class="media-browser-actions-item-name">
<strong>joomla_black.png</strong>
</li>
<li>
<button type="button" class="action-preview" aria-details="preview-desc">
<span
class="image-browser-action icon-search-plus"
aria-hidden="true"
></span
><span class="action-text">Preview</span></button
><span class="sr-only" id="preview-desc"
>Preview item: joomla_black.png</span
>
</li>
<li>
<button
type="button"
class="action-download"
aria-details="download-desc"
>
<span
class="image-browser-action icon-download"
aria-hidden="true"
></span
><span class="action-text">Download</span></button
><span class="sr-only" id="download-desc"
>Download item: joomla_black.png</span
>
</li>
<li>
<button type="button" class="action-rename" aria-details="rename-desc">
<span
class="image-browser-action fa fa-i-cursor"
aria-hidden="true"
></span
><span class="action-text">Rename</span></button
><span class="sr-only" id="rename-desc"
>Rename item: joomla_black.png</span
>
</li>
<li>
<button type="button" class="action-edit" aria-details="edit-desc">
<span
class="image-browser-action icon-pencil-alt"
aria-hidden="true"
></span
><span class="action-text">Edit</span></button
><span class="sr-only" id="edit-desc">Edit item: joomla_black.png</span>
</li>
<li>
<button type="button" class="action-url" aria-details="share-desc">
<span class="image-browser-action icon-link" aria-hidden="true"></span
><span class="action-text">Get link</span></button
><span class="sr-only" id="share-desc"
>Get a direct link to joomla_black.png</span
>
</li>
<li>
<button type="button" class="action-delete" aria-details="delete-desc">
<span class="image-browser-action icon-trash" aria-hidden="true"></span
><span class="action-text">Delete</span></button
><span class="sr-only" id="delete-desc"
>Delete item: joomla_black.png</span
>
</li>
</ul>
</div>
Which (without the fancy css and js) is rendered as
Here we can see the following issues
If we look at the suggestion from @stevefaulkner it has the same/similar issues. (probably because steve was not aware of the full context.
please remember that NO list is ever in the dom until the action button is selected. So if assistive tech is used to display a list of links on the page there will not be multiple delete buttons etc.
aria-describedby -> I read it is possible to reference an element even if that element is hidden.
If it is in the dom then yes. the class sr-only, as used in joomla only visually hides,
personaly I wonder if this really should be a menu and not a list as it appears to perfectly satisfy the description of a menu (not to be confused with a navigation menu)
@brianteeman in the case you describe I would suggest:
<button type="button" class="action-delete" aria-labelledby="delete-desc">
<span class="image-browser-action icon-trash" aria-hidden="true"></span>
<span class="action-text">Delete</span>
</button>
<span class="sr-only" id="delete-desc">Delete item: joomla_black.png</span>
or
<button type="button" class="action-rename" aria-label="Delete item: joomla_black.png">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Delete</span>
</button>
Yes, in fact for the pattern i described to work as intended the description must be hidden using
display:none
orvisibility:hidden
otherwise the desc text will be included in the accessible name as well.
Our .sr-only class has NO display:none and NO visibility: hidden
`{
position: absolute !important;
width: 1px !important;
height: 1px !important;
padding: 0 !important;
margin: -1px !important;
overflow: hidden !important;
clip: rect(0, 0, 0, 0) !important;
white-space: nowrap !important;
border: 0 !important;
} `
so that the content is part of the accessible name
<button type="button" class="action-rename">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
<span class="sr-only" id="rename-desc">item: joomla_black.png</span>
</button>
Expected Result: Rename item: joomla_black.png
This is shows a good alternative no CSS is needed and aria-label is well supported:
<button type="button" class="action-rename" aria-label="Rename item: joomla_black.png">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
</button>
aria-label wins over the button content
Expected Result: Rename item: joomla_black.png
In all it can look like:
<ul>
<li>
<button type="button" class="action-rename" aria-label="Preview item: joomla_black.png">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Preview</span>
</button>
</li>
<li>
<button type="button" class="action-rename" aria-label="Download item: joomla_black.png">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Download</span>
</button>
</li>
<li>
<button type="button" class="action-rename" aria-label="Rename item: joomla_black.png">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Rename</span>
</button>
</li>
<li>
......
</ul>
I still say item is a useless word
Thank you everyone for the ongoing discussion here. :) Pleasantly surprised to see @stevefaulkner in our little corner of Github! Thank you for your time and feedback.
One more thing that has not been noted but may be important for context:
This list of actions only shows up when triggered by an action from the user on the following button:
<button
type="button"
class="action-toggle"
tabindex="0"
aria-label="Manage item: joomla_black.png"
title="Manage item: joomla_black.png"
>
<span class="image-browser-action icon-ellipsis-h" aria-hidden="true"></span>
</button>
(now that I'm looking at it, I should probably also remove that tabindex attribute...)
@brianteeman is probably right that this should be a menu and not a list of buttons, but I'm focusing on incremental changes since there's some heavy tech debt here with how it's built.
So I can see it's pretty clear that that aria-details
was the wrong attribute to use on the button, and that we can use either aria-label
or aria-labelledby
to get the desired behavior.
One topic of ongoing discussion was whether or not each "action button" needed to repeat the filename or "item" part of the string, since the button that triggered this list to display already included the filename. Is it better to include the filename regardless via the methods you suggested on each item, or is the context provided on the button that triggered the list enough?
One of the reasons this has been a topic of discussion is both because of the verbosity and repetitiveness of each button, but also because the filename might be something long and nonsensical, like a generated alphanumeric string or something.
Thank you again for your time and input! We will put it to good use both here as well as in our developer documentation for accessible patterns :)
One of the reasons this has been a topic of discussion is both because of the verbosity and repetitiveness of each button
if there are a group (more specifically in this case it could be a toolbar of controls that all act upon a common object this could have a group label rather than repeating the object name in each description.
<div role="toolbar" aria-label="item: joomla_black.png">
<button type="button" class="action-rename">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Download</span>
</button>
<button type="button" class="action-delete">
<span class="image-browser-action fa fa-i-cursor" aria-hidden="true"></span>
<span class="action-text">Delete</span>
</button>
...
</div>
Exactly.
Super helpful. Thank you!!
Converting back to draft; It seems I have some more to do here...
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Pending |
Back to pending
Labels |
Removed:
?
|
Okay, so here is the new structure with the toolbar active:
<div class="media-browser-actions">
<button type="button" class="action-toggle" aria-label="Manage item: joomla_black.png" title="Manage item: joomla_black.png">
<span class="image-browser-action icon-ellipsis-h" aria-hidden="true"></span>
</button>
<div class="media-browser-actions-list" role="toolbar" aria-label="Item: joomla_black.png">
<span aria-hidden="true" class="media-browser-actions-item-name"><strong>joomla_black.png</strong></span>
<button type="button" class="action-preview">
<span class="image-browser-action icon-search-plus" aria-hidden="true"></span>
<span class="action-text">Preview</span>
</button>
<button type="button" class="action-download">
<span class="image-browser-action icon-download" aria-hidden="true"></span>
<span class="action-text">Download</span>
</button>
[etc...]
</div>
</div>
And here are screenshots of how it's rendered before and after clicking/triggering the toggle button:
Before I mark this as ready for testing/review, does anyone have further feedback on aria attributes, elements, etc?
I got rid of the ul
altogether since it was not serving a functional purpose. I'm not sure a span is the best element for the item name, but wanted to post it as such regardless for feedback.
You can add a role menu
You can add a role menu
Doesn't that imply/rely on other aria roles in the menu items or on the toolbar that don't currently exist? I am hesitant to add too many aria roles here since the javascript is a bit of a black box for me and I don't want to imply some kind of interaction for screen reader users that does not actually exist or work. Remember that even hover and focus on these items are being managed by javascript until I figure out how it works and can strip it down to more native html interactivity.
You can add a role menu
Doesn't that imply/rely on other aria roles in the menu items or on the toolbar that don't currently exist? I am hesitant to add too many aria roles here since the javascript is a bit of a black box for me and I don't want to imply some kind of interaction for screen reader users that does not actually exist or work. Remember that even hover and focus on these items are being managed by javascript until I figure out how it works and can strip it down to more native html interactivity.
if a role="menu"
is used the UI will need to follow the interaction requirements of an ARIA menu.
Okay. Since messing with the underlying Vue is beyond what I can do right now I am going to leave it as-is with the improved a11y commits and put this back to "ready for review".
Thank you everyone for your help!
Was it intenional to change the size of the previews?
Not so drastically. I reworked the CSS to use grid and grid-gap instead of hardcoded percentages (which left empty space to the right a lot); but I left my testing variables in there (to make sure it worked) and they slipped in. New variables committed; should be closer to before now but it depends on the screen width as the columns will auto-fit.
the keyboard up/down arrow navigation does not work - see animation of just using the down arrow
Did it work before? I definitely didn't touch that, but will take a look and see if I can figure it out.
Possibly not. But then there wasn't a role or ariavertical telling me that i should use the arrows
Yes that's what I mean. With that change I now expected the arrows to work.
Yeah, the arrows are not working because the javascript is binding to the keys instead of native interactivity. I'm working on figuring it out...
Okay, I have done what I can with the javascript there. Interactions should now follow documented toolbar behavior:
tab
and shift+tab
should move the focus in and out of the toolbarhome
key now moves the focus to the first item of the toolbarend
key now moves the focus to the last item of the toolbarall of this is based on the MDN docs: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/roles/toolbar_role
Please let me know if it works.
arrows keys work
thumbnail size - improved
Thanks
Still a lot of a11y issues to address but beyond the scope of this PR
Still a lot of a11y issues to address but beyond the scope of this PR
Don't I know it
After this latest commit I think it's ready for human tests again. I won't be tweaking CSS anymore at this point.
@nikosdion @coolcat-creations could you please retest it after the latest a11y improvements?
nvm hold off marking tests until I fix the code style stuff.
Okay, NOW we're ready for marking tests and stuff. :) thanks again everyone for your help, this is a huge improvement to the current UI.
Is that the latest build @coolcat-creations ? The language string is definitely not the most recent one and I think I had mitigated this issue already
Yes, I installed the prebuilt package update from 4.2.x to 4.2.3 with your patch. Maybe that's not the right way to do. I will investigate further.
Strange, I'll take a look. Maybe I missed something
I know. It's terrible. If you make the thumbnails bigger it's a little better and the actions are scrollable.
The current UI on mobile is not very usable either. (Not that that's an excuse!) Just have hit a wall in making it behave properly in mobile without completely refactoring the structure and how it functions.
If you want to fiddle with the CSS there to make it better you're welcome to. Major issues I was facing were overflow on the left, right, or bottom of the screen.
Once this PR is merged I will start a new one just to focus on the mobile experience because there are a few things in the media manager that could be improved.
I will be the first to admit that the current mobile experience is horrendous, but I'd like to move forward anyway in the spirit of continuous improvement (rather than delayed perfection). I think the PR goes a long way to making things better as it is, and while it doesn't make mobile better, I don't think it makes it totally worse, either. (Right now in mobile there's not much indication what each icon will do and the icons display inconsistently in my experience.)
It's a setting on screens < 576px that limits the dropdown to the size of the thumbnail, which is strange because the size could be actually full screen in theory... Its hard for me to debug because the list dissapears when I try to go into the inspector
Yes, limiting the width in small screens is intentional because otherwise it will go beyond the viewport for items to the far left or right.
It can't be full screen because the parent element has position relative set, so there's no way to reliably position the menu relative to the browser.
Removing position relative from the parent element when the menu is open causes unexpected behavior with the checkbox etc since they're positioned absolutely to the element.
It would require reworking the html and css for the entire item, basically, which may affect the vue scripts. That's why I want to do it in a subsequent PR.
Fine by me! Often PRs get stuck because of such issues. I think it would be good to move a step into the good direction and use this.
Puh Crystal , you had a lot of work. I just looked at this and even testing it is complex . I think the whole thing needs to be reviewed . I'm not sure, but maybe we should just swap out the icons for now, leaving the CSS and markup untouched. I think it needs a bigger project out of it. What do you think?
Angie, the new markup here was reviewed at length by yourself as well as Steve and Brian.
In doing so we uncovered additional issues with the JavaScript events that exist in the actions that make it completely inaccessible by keyboard or screen reader, so this is not just a visual improvement anymore but a huge accessibility improvement for this little part of the CMS.
What additional review do you feel is necessary?
@angieradtke please try to be positive. This pr has already been derailed once by a false argument - please let us not repeat that again. If you want to start from scratch then by all means do it but please dont derail this PR for something that may never happen (or not for a long time) As @coolcat-creations already said - too many pull requests stall when someone suggests a new unrelated issue and then disappears.
To quote Leonie Watson - it doesnt have to be perfect just better than it was yesterday
I have tested this item
I have tested this item
Big improvement in usability and accessibility. thank you for battling through vue to make this happen. I am sure it will be greatly appreciated!!
This is really a good work, I just thought it would be better to give us more time. Crystal and Brian , I think you know best how complex this is.
The proposed changes are important and right. But they seem to come with side effects. Brian, you are super fit with the backend. I just kept getting lost when navigating with the keyboard. Suddenly there were things on top of each other that didn't belong on top of each other.
Even small changes in the right direction are great.
I don't want to be a blocker here.
I have tested this item
I totally understand @angieradtke , and yes there are definitely other issues and problems with accessibility in the media manager. Just taking it a little at a time because iterative PRs are easier to test and merge than one massive complete overhaul. :)
Thank you for all of your input and suggestions.
@angieradtke The right way to mark a test result is to go to the issue tracker to the PR https://issues.joomla.org/tracker/joomla-cms/38771 and use the blue "Test this" button at the top left corner. Just leaving a comment with a green check mark is not sufficient. But as we have 2 other tests I can set RTC anyway.
But they seem to come with side effects.
No it doesnt. You just hadnt even tried to use it before. If you had then you would have commented much much earlier about the issues on mobile. And please remember to test everything in context and not just a snippet of code as that led to a lot of wasted time here on this pr
Status | Pending | ⇒ | Ready to Commit |
RTC
I have tested this item
I am not sure if this is the right place and time, but if you make the effort it would be useful to think about the icon. there are only images editable and here maybe the would be better. but this is just a thought.
Removing RTC
@crystalenka Could you please check the latest comments? Once they are resolved this can be merged.
Labels |
Added:
?
|
@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?
@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.
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.
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.
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.
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).
you can do that but then you can't add %s to it.
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.
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
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.
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.
@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.
@HLeithner I've added deprecation notices in the .ini files.
The old language string is also referenced here:
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 I've added deprecation notices in the .ini files.
The old language string is also referenced here:
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 ;-)
@HLeithner I've added deprecation notices in the .ini files.
The old language string is also referenced here:
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?
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
Are there any other changes you'd like me to make @HLeithner? I still see the change requested flag. :)
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-10-15 16:47:50 |
Closed_By | ⇒ | roland-d |
Thank you.
+1000 Very great improvement!