Closed (fixed)
Project:
Drupal core
Version:
10.1.x-dev
Component:
Claro theme
Priority:
Minor
Category:
Task
Assigned:
Reporter:
Created:
10 Jan 2023 at 20:45 UTC
Updated:
27 Mar 2023 at 14:30 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
hot_sauce commentedI am working on this.
Comment #4
gauravvvv commentedComment #5
gauravvvv commentedI have attached the patch and before/after screenshots for reference. please review

Before patch:
After patch:

Comment #6
aziza_a commentedChecked changes - looks good
+RTBC
Comment #8
smustgrave commentedPatch #5 appears good.
Thanks for the screenshots!
Comment #10
hot_sauce commented@Gauravvv
Thank you for the work on your patch. I was working on an MR as part of Florida DrupalCamp last week and got around to finally pushing it up today.
In comparing my MR with your patch, I think there is still room for improvement on this to help modernize the CSS more.
- For
.pager__link, .pager__item--currentwe can do more conversion to CSS Logical properties by changingmin-widthandheighttomin-inline-sizeandblock-sizerespectively- We can move the CSS variables out of
:rootand into the.pagerclass since all of these will apply to anything within pager since it's best practice to not use:rootif we don't have to- In addition, the naming conventions for the
pagervariables wasn't consistent. Some used one - and some used two, so I adjusted that for consistency sake.- We can also make CSS variables out of the background images for the
.pager__itemclasses, and then further consolidate the@media (forced-colors: active)query to use these for themask-imageas well like thisThis is, admittedly, my first contribution to Drupal directly but I think that with some tweaking between the patch and the MR we can find the best solutions here.
Comment #11
gauravvvv commentedHi @hotsaucedesign, You have assigned the issue to yourself and there were no updates for the last 5 days. So I picked up the issue. I will make sure to ask first then only I will pick issues. thankyou
Comment #12
smustgrave commentedNesting looks good. Adding some screenshots to show they have not broken anything (that I see).
Comment #13
lauriiiComment #14
hot_sauce commented@laurii
Thank you for the review, I've pushed up the fixes and commented on a couple of things, please let me know if this warrants any further work!
Comment #15
smustgrave commentedRTL seems working
Comment #17
ckrinaCommitted 30a1e23 and pushed to 10.1.x. Thanks!
Note about the unresolved comment by @lauriii about the background image: the answer given by @hot_sauce about making it easier to theme
forced-colorsmade sense to me, plus discussed with @lauriii and he's OK with the existing state.