- 🇮🇳India narendraR Jaipur, India
if it already exists on 2.0.x then would it be so bad to make it its own separate issue so this can go in?
Agreed, we can make it conditional such that if the package manager is available, the queue button is not shown, with a @todo to address it in a new issue.
in what way does it need to be refactored?
I think as we are not using ModulePage now(moved in popup), any code related to it can be removed from App.
- 🇺🇸United States chrisfromredfin Portland, Maine
@narendrar - if it already exists on 2.0.x then would it be so bad to make it its own separate issue so this can go in?
But first of all, do you believe it's ready if ModulePage is removed and App is "refactored" (in what way does it need to be refactored?)
Automatically closed - issue fixed for 2 weeks with no activity.
-
grimreaper →
committed db7aa383 on 5.1.x
Issue #3466028 by grimreaper: Fix non media library modals.
-
grimreaper →
committed db7aa383 on 5.1.x
- 🇫🇷France Grimreaper France 🇫🇷
Merged.
Follow up issue for the buttons not updated 🐛 Modal buttons not updated Active .
-
grimreaper →
committed 9ea1bc60 on 5.1.x
Issue #3466028 by grimreaper, nod_: Ensure media library popin is usable
-
grimreaper →
committed 9ea1bc60 on 5.1.x
- 🇮🇳India narendraR Jaipur, India
One issue I noticed is that clicking the Queue button in the popup does not change it to Dequeue.
This issue also exists in 2.0.x on going to detail page, but as we are removing details page with popup in this issue, I think it should be addressed here. - 🇺🇸United States phenaproxima Massachusetts
Some minor stuff, and there are out of scope changes that should probably be reverted. But overall I don't see any major problems with this code.
- 🇮🇪Ireland lostcarpark
I've added a test to simulate opening a modal detail page for a module, closing it, opening then closing the detail for a different module, and finally reopening the first module's modal.
All tests are passing.
- 🇺🇸United States chrisfromredfin Portland, Maine
re-marking for stable-blocking then.
- 🇺🇸United States phenaproxima Massachusetts
I don't think this should be a beta blocker.
That's not to say the status checks are slow, 'cause they are. But they're not so slow that they're, like, consuming gigabytes of RAM and all of the server's CPU resources. So this is more just something that sucks, and it does need to be addressed, but it's not a critical thing that needs to block a beta.
The tests are failing because they are not able to interact with the elements(by scrolling).I can't figure out how does the addition of one more element in the scroll list is causing this issue.
I tried adjusting the z-index for the apply and clear button's div but that also didn't help to fix the problem because as you can see the css of the apply and clear button's div is causing the issue (see the SS).For better understanding i changed the background colour.
I also tried to move the apply and clear button out of the select list(which is not actually we want) but that seems to pose a greater problem as you can see the SS.If anyone has any idea of what could be reason for the test fails or what else can i try to fix the problem, it would be helpful if they can document it on the issue or give it a try to fix the problem.
Thanks in advance.- 🇩🇪Germany rkoller Nürnberg, Germany
sounds reasonable, then i'll open a followup issue and postpone it until this one is in.
i think that would be a reasonable addition to make sure that this problem isnt reintroduced and slips in at one point in the future by accident.
- 🇮🇪Ireland lostcarpark
I had one more thought this morning, would it be worth creating a new test to open the modal, close it, and open it again, so that the use case of reopening the modal details is in an automated test?
- 🇮🇪Ireland lostcarpark
I had a look at the "View commands" popup, and it comes from a different component. I feel that either changing the width or adding an aria tag would be outside the scope of this issue.
I have adjusted the height to 50vh and also the spacing between the buttons as mentioned in #17 📌 Consider adding Apply and Clear buttons to Categories dropdown Active . Also i the concern raise by @rkoller in #18 📌 Consider adding Apply and Clear buttons to Categories dropdown Active is fixed related to
last visible option fully visible
along with the black background colour for the div containing the 2 buttons (which matches the bulk operation) and now it will be easier for the user to distinguish from the actual list.
Also regarding
and in regards of the keyboard navigation. would it make sense to move the details about the keyboard navigation into #3458844: Improve keyboard navigation/general a11y for categories dropdown if the rest of this issue is ready?
I think we could do that as a part of the that issue.
- 🇩🇪Germany rkoller Nürnberg, Germany
awesome thank you! only minor details. i've tested on an instance that has the composer install deactivated. in that context i would have two questions. would it make sense to make the dialog modal opened by the
view commands
button the same width like for the detail page? or will that dialog modal be removed anyway? and the other detail for theview commands
button, i would still add the aria-haspopup attribute there as well not sure if it makes sense to open a new issue for a single line and if it wouldnt be the better choice to add it within this issue as well. .i've then retested another time and opened and closed a details dialog modal several times in a row, by the close button as well as by pressing the esc key. all fine. and the max-width of 80rem is also good. also tested narrowing down the browser window to a mobile viewport and enlarging it again afterwards, all behaving well and as expected (tested in safari, firefox and edge) .
there is only one detail i wanted to note and one issue i ran into but am unable to reproduce. by default the first focusable element of the dialogue modals main content is getting into focus. problem is some projects dont have any links in their description so the first element in focus is the view commands/queue/add to batch button. so screenreader users without the visual context "might" miss the description, plus the first focusable element would be the install or the install instructions. usualy that should be the last step after a user has informed themeselves and got convinced the module is the one they wanted to test or have - but from my perspective this is not necessarily about moving the details page into a dialogue modal but more about how the details page is structured so i would vote for taggling that detail in a dedicated followup issue cuz the details page needs some work anyway imho.
the other detail was related to the problem i'Ve outlined before and i went ahead and simply tried VO-a for announcing the entire pagesequentially. problem was, voiceover after finishing the content for the first time jumped right outside of the dialogue modal and announced, i "think" it was the categories from three different modules, and then jumped right back into the dialog modal again. not reproducible so far so i wouldnt hold back the issue because of that.
so from my point of view the only questions left to answer within this issue are are about the
view commands
dialogue modal. should those details dealt within this issue or moved to a followup? aside that everything looks good to go from my perspective if there arent any objections on the code end. - 🇮🇪Ireland lostcarpark
Added "max-width" of 80rem to
project-browser-popup
class.Added
aria-haspopup="dialog"
to the project button.All tests now passing.
I think this is ready for review.
- 🇩🇪Germany rkoller Nürnberg, Germany
one detail i forgot, whichmight be reasonable to add within the scope of this issue, it would be good to add an
aria-haspopup="dialog"
attribute to the link opening the dialog modal on each of the module cards (see haspopup.mp4 - added that just via the safari web inspector for that single element) - 🇩🇪Germany rkoller Nürnberg, Germany
hm not sure if this issue is ready for rtbc. i read through the entire issue. the feedback from #6 ✨ Improve iconography usability by adding a legend Needs work in regards of the microcopy for the label by drumm wasnt really addressed , actively maintained is still sort of being used. #29 ✨ Improve iconography usability by adding a legend Needs work i cant recheck right now cuz i am unable to see any search results, but since this issue adds the second icon on the card if it applies i meanwhile think it is not out of the scope for this issue. and #36 ✨ Improve iconography usability by adding a legend Needs work isnt answered yet either about the general question of proximity. and now looking at it i wonder if the labels for those legends should be also hidden from screenreader users? they have the those legends on every card it applies to. in the context of the legend it provides no help in particular because the icons are tagged as decorational?
- 🇺🇸United States chrisfromredfin Portland, Maine
A couple of minor issues with the class names and CSS var usage. Otherwise good to go!
- 🇮🇪Ireland lostcarpark
I believe I've identified the cause of the problem that was causing the modal dialogs for each module fail to open a second time.
In Project.svelte the
<div>
for each modal was being created when the component initialized.In the handler for the module title, the
<div>
was being opened in a modal.In the close handler, the element was being destroyed.
When the title was clicked again, the element no longer existed, so it couldn't be displayed in the modal.
The fix is to move the creation of the
<div>
element into the click handler for the module card title. This allows it to be recreated every time the title is clicked.This also means the element isn't created until it's needed, or never if the titles aren#'t clicked, removing some processing from the page load.
- 🇩🇪Germany rkoller Nürnberg, Germany
in regards of aesthetics two details. first, i am unable to test the current state since it is broken or at least the buttons are not showing anymore at the moment, but in my previous tests i'Ve noticed a regression that is illustrated in #15 📌 Consider adding Apply and Clear buttons to Categories dropdown Active . we ve agreed on a previous issue that the user should always see only the half of the last visible option if there are more options than vertical available space. that way the user directly knows the list is scrollable. in the screenshot in #15 you see the last visible option fully visible instead. that way it is not directly apparent that the list is visible.
and i wonder if it would make sense to align the background color of that box/bar that contains those two buttons with the dark of the bulk action bar? to visualize that this is a button bar and not part of the multi select list anymore, to make it visualy distinguished from the actual list?
and in regards of the keyboard navigation. would it make sense to move the details about the keyboard navigation into 📌 Improve keyboard navigation for categories dropdown Active if the rest of this issue is ready?
- 🇺🇸United States chrisfromredfin Portland, Maine
@utkarsh_33 With many, many apologies, I think I really beefed this. I tried to rebase, which of course meant force-pushing. I then tried to restore, but I think my restore point was prior to these final fixes. If you still have the latest you did locally, can you please force-push it?
Changes remaining that I want:
- max-height of the dropdown should be "50vh" and not a pixel-based value
- Apply and clear buttons should not be space-between, but right next to each other, with clear being 0.5rem away (get rid of justify-content and replace with "gap: 0.5rem;" should do it.That's it for aesthetic fixes; but then we need to address the a11y of keyboard navigation. I THINK the right/logical thing would be IF it's open, we should be able to tab to the apply/clear buttons. If they're closed, tab should jump to the next filter. (??? ¯\_(ツ)_/¯ )
- 🇺🇸United States markfelton
I've reviewed #37 and the alignment of the legends and placement of the icons looks right to me desktop/tablet/mobile views.
- 🇩🇪Germany rkoller Nürnberg, Germany
One other aspect i've completely forgot about testing and mentioning is the support of home and end keys as well as the support of the page up and page down keys. both work when the multi select is expanded and in focus. but the detail that is not working is, that the checkbox focus remains on the access control checkbox. confusing for a keyboard user. in particular if you press the end key after the multi select got expanded and then you press the up key. you dont see any focus outline until the first checkbox is reached. and if you then press arrow down the focus outline moves and follows the keyboard input. the point about home and end buttons was raised by @drupa11y.
- 🇺🇸United States kd_ace Oklahoma
Updating svg legend icons viewBox values to align images when stacked vertically.
- 🇩🇪Germany rkoller Nürnberg, Germany
hm i am still in doubt about this change. visually it looks ok with the recent changes on the MR. but my problem is the spatial separation. within the filter field set you have the select list fields where you are able to set those two filter criterias in question. the labels used in the filter filed set differ from the wording on the legend (for example "security advisory coverage" against "covered by the drupal security team"). and then you have those icons on the module cards without a label. visually the user has to jump inbetween those three positions the first few times. and in the long run that legend becomes obsolete for people familiar with the information the two icons convey. and the browse page is already packed with information.
- 🇺🇸United States kd_ace Oklahoma
I have tested that spacing change in v11.0.1 by bhogue and it is good to go. The change added some needed spacing between the # of results text and the legend icons. I did notice that when stacked vertically, the green icon is not aligned properly. I will add a fix for this soon so the icons are aligned.
I just added a little spacing between the results and the icons when stacked. They looked a little squished.
- First commit to issue fork.
- 🇮🇪Ireland lostcarpark
I did some minor tweaking of the legend, wrapping them in a
<div>
to group them together.In desktop view, this groups them together cetnrally, which I think looks better:
In tablet view, the module total is on one line, and the legend on the next:
Finally in mobile view, the the number of modules, and both legend items are each on separate lines:
I think this is ready for review now.
- 🇮🇪Ireland lostcarpark
I've tweaked the layout to put the legend between the number of results and the grid/list buttons:
On mobile devices, it puts each item on its own line:
However, on small tablets, I think it needs some work:
I'll work on it some more, but feedback on work so far would be appreciated.
- 🇩🇪Germany rkoller Nürnberg, Germany
i've raised my concern i had with the current direction of first removing the drop button, moving to an add and install button solution and now moving even one step further and queuing several module to add and install. definitely convenient and streamlined for a big group of people. but that approach has one downside. from my personal perspective i have always a really really hard time figuring out after a module is installed where i am able to find configuration pages for that very module. the more module install at once it multiplies the struggle and cognitive load exponentially for me. that is the reason and rule of thumb, i also add several contrib modules but i never ever install more than one module at once. that is also the reason i am not a fan of a prebuild setup like drupal_cms where you not only have to figure out where all the configuration pages for all the installed modules are but also how they are configured. intimidating. and i suppose there are more people similar like me with similar concerns. but i think i might have an idea to to tackle the problem, without the need of re introducing drop buttons and complicate the entire flow further. i would suggest to have more actions in the bulk action list:
at the moment there is only the "add and install" action i suppose, but i would also an "add" action .with the add action people are able to select a set of modules upfront and then only composer require them . afterwards it is completely up to their preference how to proceed. and that way you would also have the flexibility. for example if you already know a certain set of module and you are familiar with their configuration you could also still choose the default "add and install".
- 🇩🇪Germany rkoller Nürnberg, Germany
well i think i know what we've both missed. i suppose you are testing with either firefox, chrome, edge or something alike, aren't you? ;) the videos i've done in #20 📌 Improve keyboard navigation for categories dropdown Active were created in safari (which is the default browser i use on macos). i've created a comparison video (comparison.mp4) with safari 18.1 on the left, firefox 132.0.2 in the middle and edge 130.0.2849.80 on the right on macos 14.7.1
so based on the points in #20 📌 Improve keyboard navigation for categories dropdown Active i went through everything again,and tagged the given points either "working" (either fixed or a none issue) or NW (needs work).
voiceover switched off
1. safari: working firefox: working edge: working
2. safari: working firefox: working edge: working
3. safari: NW firefox: NW edge: NWvoiceover switched on
1&2. safari: working firefox: working edge: working
3. safari: working firefox: working edge: working
4. safari: NW firefox: working edge: working
5. safari: NW firefox: NW edge: NW