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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cr0ss’s picture

I'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

Bojhan’s picture

Assigned: Unassigned » jessebeach

Not sure if this is by design, assigning to jesse

cr0ss’s picture

FileSize
54.42 KB

Attaching screenshot.

Sam152’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Sam152’s picture

Issue summary: View changes

This 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Looks like this is more accurately "needs review." I've left Jesse a tell on IRC as well.

jessebeach’s picture

Status: Needs review » Needs work

I like the end result of the changes. I just have a few questions about the code.

  1. +++ b/core/modules/toolbar/css/toolbar.icons.css
    @@ -283,19 +283,21 @@
     [dir="rtl"] .toolbar .toolbar-icon.toolbar-handle:before {
    -  left: auto;
    -  right: 0;
    +  left: 0;
     }
     [dir="rtl"].no-svg .toolbar .toolbar-icon.toolbar-handle:before {
    -  left: auto;
    -  right: 0;
    +  left: 0;
    

    I don't believe this left: 0 is necessary. Can you verify that?

  2. +++ b/core/modules/toolbar/css/toolbar.icons.css
    @@ -283,19 +283,21 @@
    +}
    +[dir="rtl"] .toolbar .toolbar-icon.toolbar-handle {
    +  padding-right: 0;
    +}
    +[dir="rtl"].no-svg .toolbar .toolbar-icon.toolbar-handle {
    +  padding-right: 0;
    

    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.

Sam152’s picture

This 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.

Sam152’s picture

Status: Needs work » Needs review
jorgegc’s picture

Status: Needs review » Reviewed & tested by the community

Looks good on Chrome and Firefox on OSX both LTR and RLT.

RTBC++

jessebeach’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/toolbar/css/toolbar.icons.css
@@ -269,19 +269,15 @@
 [dir="rtl"] .toolbar .toolbar-icon.toolbar-handle:before {
   left: auto;
-  right: 0;
 }
 [dir="rtl"].no-svg .toolbar .toolbar-icon.toolbar-handle:before {
   left: auto;
-  right: 0;
 }

Sorry 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.

Sam152’s picture

Status: Needs work » Needs review
FileSize
881 bytes

Good catch! Should have seen that, thanks for the review. Another patch is attached. Can confirm things are working LTR and RTL.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

Great, 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.

  • Commit 08c18e1 on 8.x by alexpott:
    Issue #2103753 by Sam152, cr0ss: Icons in toolbar are slightly...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 08c18e1 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.