- Issue created by @rkoller
- First commit to issue fork.
- 🇩🇰Denmark ressa Copenhagen
Here's an MR which increases the target size to 24px for Token icons and links. I updated the arrows, making them slightly larger, to match the larger target.
I also increased with size of the window, since it's quite small. Or should that happen in another issue? My thinking is that the size was set more than ten years ago (June 2014) and screen sizes have increased for most users. I always need to resize it.
- 🇩🇪Germany rkoller Nürnberg, Germany
Thank you for the MR! the target sizes look good. i've just checked with the bookmarklet by steven falkner that is linked in the issue summary. the only "minor" nitpick, the upper border of focus outline for the first two elements is hidden in part underneath the table header:
In regards of the font size. i am not sure if it would be ok for the maintainers to extend the scope. from my perspective it would be ok. i agree that the current font size is definitely smallish. but question is if that kind of styling falls into the changes going in version 2.x? i am not the person to decide.
and one interesting tidbit even though the typography in all three columns has the same font size (16px) it seems like the actual tokens in the second column have somehow a bigger font size :Di'll set the issue back to needs work because of the focus outline which ideally should not be covered/obscured by anything. hope that is technically possible?
- 🇩🇰Denmark ressa Copenhagen
Thanks for the review @rkoller, and generally all your great work with Accessibility in Drupal, I very much appreciate it!
About font sizes ... you're right. The link size was bigger than the rest of the text ... I thought about it, and tried using padding instead, to increase the target size. And let's see what the admins thin about increasing the font size from 0.85em to 1em.
About the overlap, I can't replicate it ... it looks like the text is top-aligned in your picture. It looks fine in my Firefox, but maybe you are using another browser?
- 🇩🇪Germany rkoller Nürnberg, Germany
oh and the link size was bigger? it looked bigger but the last time i've checked the font sizes i'Ve found for the different columns were consistent (no idea why and where i've looked lol). but looks good after your last changes. with the typography slightly bigger it would be also easier to read for everyone. it would be ok from my end to keep it and lets see what maintainers think about it.
In regards of the overlap, i've recorded a short video, on the left is the latest safari 18.1 in the middle the latest version of firefox and on the right the latest version of edge, happens in each of the three browsers for me. and i am on macos sonoma 14.7.1. you are on another operating system perhaps (which might explain the different display)?
- 🇩🇰Denmark ressa Copenhagen
Sounds great with the larger font, let's see what the maintainers say.
About the overlap. It's because all text strings are pushed to the top in Drupal 11 Claro, including the table headers "Name", "Token", and "Description" ... If you check the patch (click plain diff at the top), it should not affect the table headers.
It turns out that the cause was a recent update to Claro, aligning td's and th's vertically at the top. I didn't see it, because I wasn't using the latest Drupal 11 dev (nor Token, actually)... Lesson learned: Always use dev-version :)
I updated the patch, and it seems fine in Drupal 11 now, but maybe you can check? Thanks!
- 🇩🇪Germany rkoller Nürnberg, Germany
nice you were able to dig up the issue that was causing the problem! I can confirm the focus outline is not obscured anymore in the first row neither. thank you!
i went ahead and manually commented out the four lines listed in the corresponding core commit https://git.drupalcode.org/project/drupal/-/commit/a11036104f from the issue you've reference ✨ Add vertical-align: top as default for table cells Active . Just wanted to test if it has any negative effect if those changes get temporarily rolled back. The focus outline is still not getting obscured again for the first row. So i think this MR looks good to go. - 🇩🇰Denmark ressa Copenhagen
You're welcome, it's great that it looks good everywhere now :) About finding the problem, Gitlab is great like that -- I just had to find out which CSS file caused the problem (claro/css/components/tables.css) and then look under"History" for that file:
https://git.drupalcode.org/project/drupal/-/commits/11.x/core/themes/cla...
As you can see, the update was just reverted by @nod_, so an alternative solution will have to be found for the problem of big pieces of content pushing down some fields ... We could remove the
vertical-align: middle;
but it's probably safest to keep it in case other admin themes in the future does something similar ...Thanks for fast and great feedback, have a nice day.
- Status changed to RTBC
6 months ago 11:08pm 4 December 2024