Change .append(' ') with CSS style in system.admin.css

Upon manual review patch looks good to me, Attached are the screen shots.

checked for the following

1. Different screen sizes
2. Different types of fields, taxonomies, content types.

#1 Works for for me

Why not append the SPACE ?

Looking at the change record for "Use [dir="rtl"] in stylesheets" https://www.drupal.org/node/2032405 it looks like the CSS has been written appropriately, with the very small exception of whitespace in the LTR comments.

New patch to address that very minor whitespace in the CSS comments issue.

Other than that, RTBC.

#5 is diff to my one on #3. Back to Needs review for another reviewer.

My last testing on #3 Patch is working on RTL as well. Personally, I love my patch, more simpler :)

Thanks.

@droplet it seems redundant to me to be moving the addition of whitespace characters from the JS to the CSS. If this is a layout issue, I'd have thought it better to do the layout via margins / box model rather than the addition of space characters to the HTML, regardless of the mechanism by which they're inserted between pieces of content.

However, it's been a while since I did much front-end.

Also, your #3 patch removed the space being appended before the preview:

-        $suffix.append(' ').append($preview);
+        $suffix.append($preview);

...but this is not added back in by CSS. Instead you've added it to string after the closing span tag:

-          $suffix.append(' ').append('<span class="machine-name-label">' + options.label + ':</span>');
+          $suffix.append('<span class="machine-name-label">' + options.label + ':</span> ');

To me that reinforces the suggestion that your approach is just rearranging the way that whitespace characters are added between the text elements.

Therefore the original approach of removing the whitespace characters altogether and implementing the layout via "pure" CSS would get my vote.

I think this is at needs work per #7. I've taken a stab at the beta phase evaluation in the issue summary.

Er, wait, I got confused. This is still at needs review. I tend to agree that #5 makes sense, rather than adding more spaces in the JS as #3 does.

Rerolled.

errors on machine name, please check: admin/structure/types/add

#13 looks fine to me. Patch applies cleanly and no errors on machine name at admin/structure/types/add.

*** To remember clean caches after patching

@droplet, thank you.

Removing float seems to fix the issue. Patch attached.

although i like my own way, it's patch still looks good and no reason not let it go ahead :)

Thanks for the RTBC.

Regarding #3,

@@ -79,7 +79,7 @@ div.admin .expert-link {
  * Quick inline admin links.
  */
 small .admin-link:before {
-  content: '[';
+  content: ' [';
 }

IMO the space before [ will certainly make someone wonder so I prefer not to go for it.

It's a re-roll, passed, and was RTBC-ed before, so re-up RTBC.

Passed again, RTBC again.

I'm not that convinced by this issue. We're building text here that is meant to be read as a sentence - the space has a semantic meaning when placed between words.

I think the use of .append(' ') is acceptable and denotes that it is intended - since it would be possible to add the space into the variables if we want.

Tempted to mark "Closed - works as designed" but will punt back to needs review for further consensus building.

I'm not comfortable with creating a space using margins (partly due to the fact that I'm not sure we can replicate exactly the size of the space character with margins).

I like the solution in #3 as well, the extra characters are added from there as well, makes sense to have the spacing done at the same place.

Yeah the solution in #3 makes more sense to me. The space added in CSS should get picked up by screen readers.

The patch in #3 still applies so knocking back to RTBC so Alex can weigh in.

append the .append(' ') for the core/misc/machine-name.js

Setting back to needs review then.

what is #32 trying to do ? uploaded a wrong patch ?

Can some one upload the patch in #3 again so the correct path is the most recent patch on the issue.

Reup of #3 with one small change:

Before

$suffix.append('<span class="machine-name-label">' + options.label + ':</span> ');

After

$suffix.append('<span class="machine-name-label">' + options.label + ': </span>');

Notice the space at the end is inside the span, which means that append will only add one DOM element, not two — a span and text element.

CSS and JS changes are not subject to beta evaluation and this change is not disruptive. Committed 58a891f and pushed to 8.0.x. Thanks!