-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[4.1] Refactor vue browser items #35887
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
[4.1] Refactor vue browser items #35887
Conversation
…nager-action-items
… into j4/refactor/vue/browser
...mponents/com_media/resources/scripts/components/browser/actionItems/actionItemsContainer.vue
Show resolved
Hide resolved
...mponents/com_media/resources/scripts/components/browser/actionItems/actionItemsContainer.vue
Show resolved
Hide resolved
|
@laoneo System test is failing, I've tried to restart several times now but it always fails at the same place, so maybe it's related to this PR: https://ci.joomla.org/joomla/joomla-cms/48082/1/22 It always fails at the test "Test that it is possible to navigate to a subfolder using double click.". The screenshot from the test doesn't show anything special: https://ci.joomla.org/artifacts/joomla/joomla-cms/4.1-dev/35887/system-tests/48082 |
|
I have tested this item ✅ successfully on 3379fcc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35887. |
|
I have tested this item ✅ successfully on 3379fcc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35887. |
|
RTC |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35887. |
|
Note for maintainers: When merging with a squash commit, don't remove the "co-authored by" tags in the commit description generated by GitHub so that the author of the original PR is not forgotten. |
|
Can someone do some quick a11y validation on this one. I assume as we still have the unordered list it's ok. But some of the html here was refactored from divs to ul's originally for proper screenreader compat |
|
@brianteeman can you have a look on the concern from @wilsonge? |
|
Please ask the accessibility team |
|
I will ping them on Glip. |
|
@richard67 thanks |
@wilsonge there are no structural changes here. The same HTML markup just moved around. (or am I wrong?) |
|
@dgrammatiko The thing is that while this PR was in progress, changes on the code which is moved around by this PR have been made, and so there were conflicts which were resolved. I think George just wants to be sure that the other changes have been applied to this PR when the conflicts were solved. |
|
The conflicts were created by me as by #35451. I'v fixed them already. There were no structural changes involved in the other pr. I had a second look here and indeed, there are no structural changes as far as I can see, it was before also a list. |
|
I can confirm there are no html structural changes from an a11y point of view. Whenever there should be a list, there is a list and no divs are involved. Also with this refactoring, checking the code and A11y fixes will be simpler as we will split longer code in vue components reusing the markup. I do think this is a RTC!! |
|
Is is RTC already. |
|
Thanks @carcam !! |
|
Thx |
Followup pr of #35332.
Summary of Changes
As #35332 gets abandoned, this one here is a conflict fix. All the old commits should be preserved.
More information can be found in the pr #35332 by @JenSeReal.
Testing Instructions
Open media manager and execute the actions and browse around.
Actual result BEFORE applying this Pull Request
All should work.
Expected result AFTER applying this Pull Request
All should work.