Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tsi’s picture

Status: Active » Needs review
FileSize
2.98 KB
Gábor Hojtsy’s picture

Issue tags: +D8MI
mjohnq3’s picture

This looks good except for one minor problem - no left padding on the main menu tabs. See attached screenshot and interdiff.txt.

tsi’s picture

The padding there should be (according to the LTR styles) padding: 0.7em 0.8em; and apply only under the min-width: 901px media query, this means we add a line to the RTL styles which looks the same as the LTR equivalent only for cascading reasons (patch attached). not perfect but maybe we have no choice.

Status: Needs review » Needs work

The last submitted patch, bartik_menu_tabs_rtl-1994192-4.patch, failed testing.

mjohnq3’s picture

Status: Needs work » Needs review
FileSize
672 bytes
2.6 KB

I'll take another crack at this. The affected css is now mobile-first.

tsi’s picture

There's a tab that snuck in there.
Besides that, seems like #6 does the same as #4 only that it is formatted better (mine got messed up by git) so it is fine by me.
This one removes the tab.

thamas’s picture

Status: Needs review » Reviewed & tested by the community

Tested the last patch, it works well.

Dries’s picture

Priority: Normal » Minor
Status: Reviewed & tested by the community » Needs work

Committed to 8.x. but I'd like us to clean up the comments. They are not very consistent.

xjm’s picture

+++ b/core/themes/bartik/css/style-rtl.cssundefined
@@ -70,9 +70,11 @@ ul.tips {
 /* --------------- Secondary Menu ------------ */

@@ -249,6 +251,45 @@ ul.tips {
+/* ----------- media queries ------------------------------- */
...
+ /* ------------ Header and Menus -------------------------- */

To clarify @Dries' comment, the new hyphen-header things have more hyphens and the case is not consistent.

mjohnq3’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

The hyphen-header thingies are now all title case and start at column #24. The ending "/" all end at column #62.

Status: Needs review » Needs work
Issue tags: -frontend, -D8MI

The last submitted patch, bartik_menu_tabs_rtl-1994192-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +frontend, +D8MI
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b3b93ac and pushed to 8.x. Thanks!

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