Problem

Content has a margin on the right proper, but right floated sidebar container will not appear on the right, upsetting the whole page structure.

Only local images are allowed.

Files: 
CommentFileSizeAuthor
#19 menu-less-than-851px.jpg69.45 KBmjohnq3
#19 menu-less-than-560px.jpg53.17 KBmjohnq3
#6 fix-broken-layout-rtl-1979392-5.patch2.84 KBthamas
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es).
[ View ]
#5 Screenshot_4_26_13_8_29_AM.png67.52 KBGábor Hojtsy
#5 Screenshot_4_26_13_8_33_AM.png46.19 KBGábor Hojtsy
#4 fix-broken-layout-rtl-1979392-3.patch549 bytesthamas
PASSED: [[SimpleTest]]: [MySQL] 55,475 pass(es).
[ View ]
#3 fix-broken-layout-rtl-1979392-0.patch406 bytesthamas
PASSED: [[SimpleTest]]: [MySQL] 55,343 pass(es).
[ View ]
Screenshot_4_25_13_9_54_AM.png130 KBGábor Hojtsy

Comments

Issue tags:+frontend

Assigned:Unassigned» thamas

In progress, patch coming soon…

Status:Active» Needs review
StatusFileSize
new406 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,343 pass(es).
[ View ]

Here is a basic patch which should fix the issue.

StatusFileSize
new549 bytes
PASSED: [[SimpleTest]]: [MySQL] 55,475 pass(es).
[ View ]

The patch in #3 is not right cause it places first sidebar to the left, however in RTL the first should be on the right (I think). So here is a new one which works as it should. (It is against the default state, not the patched version, so patch in commet #3 should be simply ignored. I hope I do it right…)

Status:Needs review» Needs work
StatusFileSize
new46.19 KB
new67.52 KB

Thanks! This looks good on the desktop (where the 25% margins are affective). Also cross-checking the CSS, you applied the same rules reversed, so good :)

Screenshot_4_26_13_8_29_AM.png

It does not seem to affect the responsive state in @media all and (min-width: 560px) and (max-width: 850px) where the sidebars are 50%. Just size your browser down to see:

Screenshot_4_26_13_8_33_AM.png

Also I don't think we removed the requirement to add /* LTR */ comments on properties in the original sheet which need modification in the RTL sheet so we don't forget them again like we did this time :D

In short, looks good on desktop, needs work on the smaller version and to add /* LTR */ comments to properties in the original sheet which are overriden in RTL.

StatusFileSize
new2.84 KB
PASSED: [[SimpleTest]]: [MySQL] 55,297 pass(es).
[ View ]

OK, here is the new patch to fix the issues you mention. I also made sidebar width to 100% when there is only one sidebar and screen width is below 850px. (I found a line in the code which suggested this, but it did not work: .one-sidebar #sidebar {width: 100%;}) If this is not the expected behaviour I can change this.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Looks amazing in RTL, it adapts properly to whatever screen size I put it at. Also, looks good in terms of the CSS changes themselves now reproducing the responsive sections in RTL that were sadly missed in the responsive conversion.

Issue tags:+sprint

Put on D8MI sprint for commit tracking.

Status:Reviewed & tested by the community» Fixed

Awesome work!!

This looks great, apart from some minor spacing issues I noticed /*LTR*/ -> /* LTR */ but I just fixed those on commit.

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

Woot, thanks @thamas for jumping on this.

Wooo! My first core patch!

I'm happy and proud to be able to help! :)

@thamas congrats!

What made you jump in?

@YesCT thanks!

I always want to help, but my resources (time, knowledge) are limited. I found the issue by reading Gábor's tweet about it. And I saw that I am able to fix it and found the time to do it. :)

tweeting++

:)

For when you do have time... we are starting to tag frontend like issues to make them easier to find:
http://drupal.org/project/issues/search/drupal?status%5B%5D=Open&issue_t...

Thanks for the info! I saw the new tag in #1 but I have forgotten it since than… :)

Great work, thamas! Welcome to the team. :D

Thanks, Angie! :)

Status:Needs work» Active
StatusFileSize
new53.17 KB
new69.45 KB

Sorry, but this change completely borked the Menu tabs (when they change to buttons). At less than 851px they should be three across, each approx. one-third the width of the screen. At less than 560px they should each be the full screen width, and stacked vertically. See the attached screenshots.

I'm trying to see if I can figure out what happened.

Assigned:thamas» Unassigned
Status:Fixed» Needs work

Title:Bartik broken in RTLBartik layout broken in RTL
Status:Active» Closed (fixed)

@mjohnq3 The problem with menu tabs is not related to the changes here. The bug was there before the patches (I just tested that state). And I feel we will find other RTL issues as well. (We should check all the stylesheets for possible RTL problems: floats, different margins…)

So I think we should open a new issue for the menu tabs in RTL just to keep things more clear.

Let's open a new issue and link it from here indeed! @mjohnq3: are you up for that?

@thamas - Yes, I can now see that this change didn't affect the menu problem.

@Gabor Hojtsy - I'll take a look as soon as I can. I had a hard drive crash and also was without internet access for several many hours.

Reference - new issue for the menu fix: #1994192: Bartik's menu tabs in RTL

@thamas: maybe you can help review/test #1994192: Bartik's menu tabs in RTL? :)

@Gábor Hojtsy I'll do my best… :)