Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Right now I'm seeing the icons being a liltle misaligned in both LTR and RTL(to the very left and very right side of their outline box).
Attached patch solves this issue, please review.
Comment | File | Size | Author |
---|---|---|---|
#12 | 2103753-12-misaligned-toolbar-icons.patch | 881 bytes | Sam152 |
#8 | 2103753-8-misaligned-toolbar-icons.patch | 882 bytes | Sam152 |
#3 | toolbar.png | 54.42 KB | cr0ss |
toolbar.icons-css.patch | 1.04 KB | cr0ss | |
Comments
Comment #1
cr0ss CreditAttribution: cr0ss commentedI'm having issues with attaching screenshot right now due to some validation issue d.o is throwing back, but will try upload it later to provide more details on this issue.
Attaching screen by clip2net http://clip2net.com/s/5SrQ8V
Comment #2
Bojhan CreditAttribution: Bojhan commentedNot sure if this is by design, assigning to jesse
Comment #3
cr0ss CreditAttribution: cr0ss commentedAttaching screenshot.
Comment #4
Sam152 CreditAttribution: Sam152 commentedComment #5
Sam152 CreditAttribution: Sam152 commentedThis patch still applies. I would be almost certain that this is not by design and the patch resolves a bug. The equal spacing between the top and right of the icon looks a lot better than what is currently there.
Setting to RTBC unless jessebeach wants to chime in and say otherwise.
Comment #6
webchickLooks like this is more accurately "needs review." I've left Jesse a tell on IRC as well.
Comment #7
jessebeach CreditAttribution: jessebeach commentedI like the end result of the changes. I just have a few questions about the code.
I don't believe this
left: 0
is necessary. Can you verify that?What padding is this removing? Do you have a theme other than Bartik applied while you're developing? Just trying to determine why you would be zeroing out padding here.
Comment #8
Sam152 CreditAttribution: Sam152 commentedThis has been stressing out the OCD portion of my brain for ages. So, I arrived at the same end with the attached patch.
I'm not sure what the additional CSS in the previous patch was for. The attached patch was manually tested under all combinations of LTR, RLT & no-svg.
Comment #9
Sam152 CreditAttribution: Sam152 commentedComment #10
jorgegc CreditAttribution: jorgegc commentedLooks good on Chrome and Firefox on OSX both LTR and RLT.
RTBC++
Comment #11
jessebeach CreditAttribution: jessebeach commentedSorry to be a nudge about this :)
If we remove the
left:0
property from the LTR declarations, then we don't need to reset it in the RTL declarations. So, it seems like the RTL declarations here aren't even needed. And any time we can remove CSS, let's do it.Comment #12
Sam152 CreditAttribution: Sam152 commentedGood catch! Should have seen that, thanks for the review. Another patch is attached. Can confirm things are working LTR and RTL.
Comment #13
jessebeach CreditAttribution: jessebeach commentedGreat, thank you for sticking with this issue Sam152.
The updated patch realigns the icons nicely. I did some cross browser testing to verify no visual regressions.
Comment #15
alexpottCommitted 08c18e1 and pushed to 8.x. Thanks!