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.

Files: 
CommentFileSizeAuthor
#12 2103753-12-misaligned-toolbar-icons.patch881 bytesSam152
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,512 pass(es).
[ View ]
#8 2103753-8-misaligned-toolbar-icons.patch882 bytesSam152
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,811 pass(es).
[ View ]
#3 toolbar.png54.42 KBcr0ss
toolbar.icons-css.patch1.04 KBcr0ss
PASSED: [[SimpleTest]]: [MySQL] 58,865 pass(es).
[ View ]

Comments

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

Assigned:Unassigned» jessebeach

Not sure if this is by design, assigning to jesse

StatusFileSize
new54.42 KB

Attaching screenshot.

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

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.

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.

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.

StatusFileSize
new882 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,811 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

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

RTBC++

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.

Status:Needs work» Needs review
StatusFileSize
new881 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,512 pass(es).
[ View ]

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

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

Status:Reviewed & tested by the community» Fixed

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