The design of Seven's drop button widget does not look good in RTL languages.

Before patch

Before patch

After patch

After patch

CommentFileSizeAuthor
#23 dropbutton_rtl_fixes-2287781-23.patch3.84 KBherom
#23 dropbutton-fix-border-radius-rtl.png6.77 KBherom
#23 interdiff-2287781-20-23.txt709 bytesherom
#22 Screenshot 2014-07-29 16.50.35.jpg18.96 KBLewisNyman
#20 dropbutton_rtl_fixes-2287781-20.patch3.84 KBherom
#20 interdiff-2287781-19-20.txt829 bytesherom
#20 dropbutton-rtl-after.png6.69 KBherom
#20 dropbutton-rtl-before.png6.67 KBherom
#19 dropbutton_rtl_fixes_2287781_19.patch3.64 KBalayham
#14 seven_theme_dropbutton_component_2287781_14.patch3.43 KBalayham
#12 seven_theme_dropbutton_component_2287781_12.patch3.43 KBalayham
#10 Screenshot 2014-07-12 14.39.27.jpg299.11 KBLewisNyman
#8 Screenshot 2014-07-12 10.31.22.jpg33.52 KBLewisNyman
#5 dropbutton-content-types.png14.66 KBalayham
#5 seven_dropbutton_rtl_fix-2287781-5.patch1.32 KBalayham
#1 seven_dropbutton_rtl_fix-2287781-1.patch1.13 KBalayham
#1 seven_dropbutton_rtl_fix.png20 KBalayham
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alayham’s picture

alayham CreditAttribution: alayham commented
Priority: Normal » Minor
Status: Active » Needs review
FileSize
seven_dropbutton_rtl_fix.png20 KB
seven_dropbutton_rtl_fix-2287781-1.patch1.13 KB

This shot shows the dropbutton widget in views_ui after appying the attached patch, as well after applying the other 2 related patches for views_ui module: #2287381: views_ui dropbuttons display is not RTL friendly and #2286923: views_ui dropbuttons are not RTL friendly

Dropbutton after the fix

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Much better

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

We need to add /* LTR */ to the corresponding ltr css rules

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Issue tags: +CSS, +frontend 
+++ b/core/themes/seven/css/components/dropbutton.component.css
@@ -183,12 +183,25 @@
+[dir="rtl"].js .dropbutton-wrapper .dropbutton-widget .dropbutton-toggle button {
+  border-right: 1px solid #a6a6a6;
+  border-left: 0;
+}
+

We don't tend to add space between selectors unless there is a comment

Also, can we get a few screenshots of non-views pages to make sure we aren't causing regressions?

alayham’s picture

alayham CreditAttribution: alayham commented
Status: Needs work » Needs review
FileSize
seven_dropbutton_rtl_fix-2287781-5.patch1.32 KB
dropbutton-content-types.png14.66 KB

Thanks for the corrections. Here is the patch again, with a shot of the content types page.
Content types

alayham’s picture

alayham CreditAttribution: alayham commented
Status: Needs review » Reviewed & tested by the community

This is related to #2287381: views_ui dropbuttons display is not RTL friendly and should be committed as well to fix the issue.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review

Please don't RTBC your own patch. Sounds like Lewis was asking for some screenshots.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Needs work
FileSize
Screenshot 2014-07-12 10.31.22.jpg33.52 KB

Ah sorry I forgot to review this. I noticed this behaviour on hover:

alayham’s picture

alayham CreditAttribution: alayham commented
Status: Needs work » Needs review

Lewis, This behavior is something different and not related to the patch I sent. I will work on it in a different patch after the three I sent are done.
According to the policy in #1166886: [Policy, no patch] Improve RTL support

All these 7.0 and 8.0 rtl patches need to be committed once and for all, even if only 99% perfect, they are all interdependent and every time one of them gets outdated because another css patch got committed, all or many of them go back to needs work.

That's why I am pushing to get The three RTL patches for the drop button component to be committed. Two are done, and this one is remaining.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
FileSize
Screenshot 2014-07-12 14.39.27.jpg299.11 KB

You're going to have to help me understand what I'm supposed to be testing here. The screenshots in the summary and in comments show the views UI but this patch isn't affecting the views UI?

This is what the content types page looks like for me right now with the patch applied:

alayham’s picture

alayham CreditAttribution: alayham commented

Hi Lewis. There are many issues with the dropbutton component's display in Seven when using an RTL language. These issues are not related to this patch. all I did here is fix some border and border-radius css values to get it to display correctly without hover. I will work on the hover and the positioning, as well as some css overlay issues in some situation one by one, and I plan to fix them all.

I believe fixing them all in a single patch can be more difficult to maintain in the future, because if a small fix caused other issues, you can't roll back the patch, you have to fix it with a different fix. This is why I am working on small fixes and getting the display better with each patch.

here is how the dropbutton looks before this patch
Before
and here it is after the patch
after
after

There is some progress here. I fixed the separator and the border radius. That's all this what this patch does.

Do you suggest a single patch for all of the RTL issues of Dropbutton in Seven at once?

alayham’s picture

alayham CreditAttribution: alayham commented
FileSize
seven_theme_dropbutton_component_2287781_12.patch3.43 KB

This patch solves the following issues for the Dropbutton component in Seven:
- The separator border
- Style of the drop button.
- many hover issues
- positioning issues

Status: Needs review » Needs work

The last submitted patch, 12: seven_theme_dropbutton_component_2287781_12.patch, failed testing.

alayham’s picture

alayham CreditAttribution: alayham commented
FileSize
seven_theme_dropbutton_component_2287781_14.patch3.43 KB

This patch solves the following issues for the Dropbutton component in Seven:
- The separator border
- Style of the drop button.
- many hover issues
- positioning issues

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: seven_theme_dropbutton_component_2287781_14.patch, failed testing.

alayham’s picture

alayham CreditAttribution: alayham commented

I have no idea why this patch is failing the test. Any help?

$ git reset --hard
HEAD is now at 4a13614 Issue #2039449 by joshi.rohit100, joachim, lokapujya, Jalandhar, paulh, yaworsk: Fix docs for assertField methods in WebTestBase to describe how to skip value checking

$ git apply seven_theme_dropbutton_component_2287781_14.patch

$ git status
# On branch 8.x
#       modified:   core/themes/seven/css/components/dropbutton.component.css
$
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

If that's the latest commit in your repo then it looks like you might be almost a month behind HEAD. I would commit it the patch and attempt a rebase https://www.drupal.org/patch/reroll

alayham’s picture

alayham CreditAttribution: alayham commented
Title: Seven Theme DropButton component does not look well in RTL pages » DropButton component does not look well in RTL page
Status: Needs work » Needs review
FileSize
dropbutton_rtl_fixes_2287781_19.patch3.64 KB

Here are the fixes again.

I hope this will work before another css restructuring gets committed. I am having the difficulties mentioned in #1166886: [Policy, no patch] Improve RTL support

herom’s picture

herom CreditAttribution: herom commented
Issue summary: View changes
FileSize
dropbutton-rtl-before.png6.67 KB
dropbutton-rtl-after.png6.69 KB
interdiff-2287781-19-20.txt829 bytes
dropbutton_rtl_fixes-2287781-20.patch3.84 KB
herom’s picture

herom CreditAttribution: herom commented

I fixed a border issue, and updated the issue summary with new screenshots.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
Status: Needs review » Needs work
FileSize
Screenshot 2014-07-29 16.50.35.jpg18.96 KB

For some reason the primary dropdown on the content creation screen looks like this for me:

It seems to fix if I change those 20em border radius values to 1em but I'm guessing that breaks somewhere else? I think we maybe be in CSS Specificity hell...

herom’s picture

herom CreditAttribution: herom commented
Status: Needs work » Needs review
FileSize
interdiff-2287781-20-23.txt709 bytes
dropbutton-fix-border-radius-rtl.png6.77 KB
dropbutton_rtl_fixes-2287781-23.patch3.84 KB

aah, that should be fixed now. thanks for noticing.

alayham’s picture

alayham CreditAttribution: alayham commented
Status: Needs review » Reviewed & tested by the community

Works fine and solves the problem, thank you very much.

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed ced85d1 and pushed to 8.x. Thanks!

  • alexpott committed ced85d1 on 8.0.x 
    Issue #2287781 by alayham, herom: Fixed DropButton component does not...

Status: Fixed » Closed (fixed)

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