Hi Nadav,

The fix looks ok to me, but its in the wrong file. responsive.less is for the responsive design css rules.

I think this should be in core.less

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Hi Dan,

You are probably right. I was confused by the fact that it was the only place I could see some similar (".nav-tabs") CSS classes. So I placed it there. Anyways, I moved the fix into the end of core.less . Hope it is in the right place.

And rebased on latest 23-8-2013 master.
Ready for peer-review.

Hi Nadav,

thanks for fixing this. Very minor comments:

1. Unlike Dan, I think this should be in modules.less. Not because it makes sense, just because other nav elements (.navbar) have been set there already, and it's probably better to keep them all together. Though if I had to vote, I'd create a navs.less file as the one in bootstrap.
2. You have an indentation issue, mix of 2 spaces and 4 spaces.

This makes me think that fixing RTL will be an endless job. Have we considered using some RTL releases of Bootstrap?

Cheers,
Fred

Hi Dan Poltawski,

I fixed Frédéric Massart's 2nd comment. And rebased (19/9/2013)
Do you think we should also apply Frédéric Massart's 1st comment?

If not, please see if you can forward this issue to integration.

Hi Guys,

I'd go for Freds advice rather than mine.

thanks

Ok. I will move them into modules.less.
And if you all wish to clean up the theme, then we open a new issue and move all the nav elements to navs.less.
Agree?

That sounds good to me, thanks!

Nadav, I would be careful what you do if you intend cleaning up bootstrapbase/less files. Also if your fix for this is new then you should be adding it to bootstrapbase/less/moodle/undo.less which is basically un-doing bootstrap settings.

Just adding David Scotson to see what needs cleaning up with regards bootstrapbase/less files.

Mary, I am leaving the clean up to a different issue.
I am just (about) to move the fix from one file (core.less) to the other (modules.less)

I have updated the patch.
Frédéric Massart, Please review.

Hi Nadav, it looks good to me. I suppose you will provide a patch for 2.5 as well. Thanks! Let me know if I need to "click" the "Submit for integration" button.

FYI Bootstrap are working on pulling RTL into core Bootstrap for 3.1:

https://github.com/twbs/bootstrap/issues/9397

Following their lead (once they finally decide which approach to take) would be a good idea in general. Though Moodle isn't able to use straight Bootstrap as is, so it won't be able to use straight Bootstrap RTL either (whether the core Bootstrap way, or any of the several existing RTL forks), so every hack we have to do to get Bootstrap working, still requires double the work for LTR and RTL.

As for tidying up, the current .less (mostly) follows directly the Base theme's .css layout, so it would almost certainly be better if split according to actual functional lines. At the same time the last of the formatting could be sorted out too. I left the original code formatting in place until I changed stuff so I could track which bits of the file had been fixed and which bits hadn't been touched, but if people are splitting the files they should probably reformat them at the same time.

If you are tidying up then perhaps a bootstrap-rtl.less file would be appropriate, for items like this which are overriding core bootstrap. It makes more sense to me than creating one file for every original Bootstrap less file, as I imagine that most of the overrides will be RTL related. Perhaps in the short term simply grabbing one of the RTL Bootstrap forks would provide this file for you? Though again remember, the Moodle specific stuff still needs worked on, but tabs should be easily done with 3rd party code.

I suggested this previously for core Moodle RTL stuff (MDL-35084) but it was thought better to keep the stuff together. Perhaps now the core of the CSS is being brought in from a 3rd party source it makes more sense.

David, thanks for this info. To drift really far off topic for this issue (and apologies if you've already discussed this in other areas).

As for tidying up, the current .less (mostly) follows directly the Base theme's .css layout, so it would almost certainly be better if split according to actual functional lines. At the same time the last of the formatting could be sorted out too. I left the original code formatting in place until I changed stuff so I could track which bits of the file had been fixed and which bits hadn't been touched, but if people are splitting the files they should probably reformat them at the same time.

I really think that we should take the initiative and work towards getting the less files into a structured/documented/managed approach which takes full advantage of the functionality that less gives us before it becomes 'too late'. Already we've had about 30/40 different people working on these files and I can see us quickly loosing the advantage of variables/mixins as every person adds their own manual css change.

Dan, to be honest I think maybe 80% of the content in those less files taken from Base theme CSS is entirely unnecessary, so if I was to draw up a plan for getting the .less files into shape then that would be my first priority. Not only would it make the files easier to understand and work on, it'll massively reduce the size of the CSS sent to users and so speed up Moodle. I wrote up some ideas on this here: MDL-39094 (Note that point 13 is about splitting the less file, and 14 is about RTL).

As I mention in the pre-amble of that bug "every person adds their own manual css change" is basically the root of the problem, but the solution is for people to use standardized HTML and any change that adds CSS to be looked at with great suspicion. So the fix for the CSS problem needs to happen mostly in the HTML.

Frédéric Massart, I added a fix for MOODLE_25_STABLE too. Please click the "Integration" button

David, Thank you! for the update on RTL plans for future Bootstrap 3.1 (Beautiful news)

Reading back the discussion we had on MDL-35084, I still find it more easy for me to maintain all the RTL selectors when I see them immediately after the LTR selectors, in one file.

We should definitely find a way to use a preprocessor to remove all the redundant RTL or LTR selectors when serving the compiled CSS file to the browser.

I think we should continue this discussion on MDL-39094.

Thanks Nadav, pushing for integration.

Thanks Nadav, it has been integrated in 2.5 and master.

Please note that I removed comments from commits because now it is in the middle of the file and not in the end and this comment is confusing

Test passed. Thanks Nadav (" ,)

You did it!

Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.