Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +RTL
thamas’s picture

Assigned: Unassigned » thamas

Wokin' on it :)

thamas’s picture

Here is the patch which adds the missing padding and removes some unneeded lines. It (hopefully) also fixes some inconsistencines in drupal.base.css I found during working on the original issue.

Before:

content-lang-settings-rtl.png

After:

content-lang-settings-rtl-fixed.png

(Sorry for sending it so much later I assigned it to myself but DrupalCon catched me… ;))

rteijeiro’s picture

Status: Needs review » Needs work
diff --git a/core/misc/drupal.base.css b/core/misc/drupal.base.css
index 2f081e9..77e55a2 100644
--- a/core/misc/drupal.base.css
+++ b/core/misc/drupal.base.css
@@ -54,20 +54,18 @@ table {
   border-collapse: collapse;

This file doesn't exists, so patch doesn't apply :(

thamas’s picture

Issue tags: +Needs reroll

Ohh, it was removed two days later: https://drupal.org/node/1839318#comment-7912709

I'll reroll the patch… Thanks for the review!

thamas’s picture

Rerrolled without the removed drupal.base.css

thamas’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies well and code seems right. Well done!

thamas’s picture

Thanks for the review! :)

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

Sorry if this is obvious, but:

+++ b/core/modules/content_translation/css/content_translation.admin.css
@@ -6,30 +6,31 @@
+  padding-left: 1em;
...
+  padding-left: 1em;

Any reason to add this additional padding? I don't see the equivalent being added in ltr-mode.

Gábor Hojtsy’s picture

I think that would be the standard table cell padding that is overridden in the LTR version, so needs to set back again? @thamas, can you confirm?

thamas’s picture

Status: Needs review » Reviewed & tested by the community

We are overriding the 3em and 5em left padding which are defined earlier in LTR settings.

Gábor Hojtsy’s picture

Issue tags: +sprint

Tag on sprint for easier tracking.

plach’s picture

Title: Cotent translation module admin page does not support RTL » Content translation module admin page does not support RTL
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay! Thanks!

Maybe you noticed the random distracting labels right near the checkboxes on the screenshots in #3, those are *not supposed to be there*. Simple RTBC issue at #2112303: Random extra text around translatability configuration is confusing.

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