Moodle
  1. Moodle
  2. MDL-41264

Right align Tabs on Wiki (and Glossary) page, when in RTL mode (theme/clean)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.1
    • Fix Version/s: 2.5.3, 2.6
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Set language to Hebrew (or any RTL language)
      2. Navigate into a Wiki activity
      3. Make sure all "Tabs" (above the content) are right aligned.
      4. Navigate into a Glossary activity
      5. Make sure all "Tabs" (above the content) are right aligned.
      Show
      Set language to Hebrew (or any RTL language) Navigate into a Wiki activity Make sure all "Tabs" (above the content) are right aligned. Navigate into a Glossary activity Make sure all "Tabs" (above the content) are right aligned.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41264_master

      Description

      Tabs above the content on several pages in Moodle should be right aligned in RTL mode (theme/clean, theme/bootstrapbase)

      1. Wiki page
      2. Glossary page

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

            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

            Show
            Dan Poltawski added a comment - 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
            Hide
            CiBoT added a comment -

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

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Nadav Kavalerchik added a comment -

            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.

            Show
            Nadav Kavalerchik added a comment - 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.
            Hide
            Frédéric Massart added a comment -

            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

            Show
            Frédéric Massart added a comment - Hi Nadav, thanks for fixing this. Very minor comments: 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. 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
            Hide
            Nadav Kavalerchik added a comment -

            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.

            Show
            Nadav Kavalerchik added a comment - 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.
            Hide
            Dan Poltawski added a comment -

            Hi Guys,

            I'd go for Freds advice rather than mine.

            thanks

            Show
            Dan Poltawski added a comment - Hi Guys, I'd go for Freds advice rather than mine. thanks
            Hide
            Nadav Kavalerchik added a comment -

            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?

            Show
            Nadav Kavalerchik added a comment - 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?
            Hide
            Frédéric Massart added a comment -

            That sounds good to me, thanks!

            Show
            Frédéric Massart added a comment - That sounds good to me, thanks!
            Hide
            Mary Evans added a comment -

            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.

            Show
            Mary Evans added a comment - 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.
            Hide
            Mary Evans added a comment -

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

            Show
            Mary Evans added a comment - Just adding David Scotson to see what needs cleaning up with regards bootstrapbase/less files.
            Hide
            Nadav Kavalerchik added a comment -

            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)

            Show
            Nadav Kavalerchik added a comment - 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)
            Hide
            Nadav Kavalerchik added a comment -

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

            Show
            Nadav Kavalerchik added a comment - I have updated the patch. Frédéric Massart , Please review.
            Hide
            Frédéric Massart added a comment -

            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.

            Show
            Frédéric Massart added a comment - 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.
            Hide
            David Scotson added a comment -

            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.

            Show
            David Scotson added a comment - 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.
            Hide
            Dan Poltawski added a comment -

            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.

            Show
            Dan Poltawski added a comment - 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.
            Hide
            David Scotson added a comment -

            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.

            Show
            David Scotson added a comment - 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.
            Hide
            Nadav Kavalerchik added a comment -

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

            Show
            Nadav Kavalerchik added a comment - Frédéric Massart , I added a fix for MOODLE_25_STABLE too. Please click the "Integration" button
            Hide
            Nadav Kavalerchik added a comment -

            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.

            Show
            Nadav Kavalerchik added a comment - 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 .
            Hide
            Frédéric Massart added a comment -

            Thanks Nadav, pushing for integration.

            Show
            Frédéric Massart added a comment - Thanks Nadav, pushing for integration.
            Hide
            Marina Glancy added a comment -

            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

            Show
            Marina Glancy added a comment - 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
            Hide
            Barbara Ramiro added a comment -

            Test passed. Thanks Nadav (" ,)

            Show
            Barbara Ramiro added a comment - Test passed. Thanks Nadav (" ,)
            Hide
            Dan Poltawski added a comment -

            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.

            Show
            Dan Poltawski added a comment - 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.

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: