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.

Screenshot_4_25_13_9_54_AM.png

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Issue tags: +frontend
thamas’s picture

Assigned: Unassigned » thamas

In progress, patch coming soon…

thamas’s picture

Status: Active » Needs review
FileSize
406 bytes

Here is a basic patch which should fix the issue.

thamas’s picture

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…)

Gábor Hojtsy’s picture

Status: Needs review » Needs work
FileSize
46.19 KB
67.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.

thamas’s picture

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.

thamas’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

Issue tags: +sprint

Put on D8MI sprint for commit tracking.

webchick’s picture

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!

Gábor Hojtsy’s picture

Issue tags: -sprint

Woot, thanks @thamas for jumping on this.

thamas’s picture

Wooo! My first core patch!

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

YesCT’s picture

@thamas congrats!

What made you jump in?

thamas’s picture

@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. :)

YesCT’s picture

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...

thamas’s picture

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

webchick’s picture

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

thamas’s picture

Thanks, Angie! :)

mjohnq3’s picture

Status: Needs work » Active
FileSize
53.17 KB
69.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.

mjohnq3’s picture

Assigned: thamas » Unassigned
Status: Fixed » Needs work
thamas’s picture

Title: Bartik broken in RTL » Bartik 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.

Gábor Hojtsy’s picture

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

mjohnq3’s picture

@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.

tsi’s picture

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

Gábor Hojtsy’s picture

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

thamas’s picture

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