Moodle
  1. Moodle
  2. MDL-32479

RTL Theme fixes for Moodle 2 (theme/base)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Select BASE theme
      2. Switch to RTL language
      3. Turn on editing and check that block actions are aligned nicely.
      4. Check the layout of a file picker element anywhere
      5. Check the layout of a mform with a fieldset element (a grouping)
      6. Check the alignment of block headings on my/index.php
      7. Add a combo list to the front page and check its alignment
      Show
      Select BASE theme Switch to RTL language Turn on editing and check that block actions are aligned nicely. Check the layout of a file picker element anywhere Check the layout of a mform with a fieldset element (a grouping) Check the alignment of block headings on my/index.php Add a combo list to the front page and check its alignment
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-32479_master
    • Rank:
      39369

      Description

      Support for blocks columns switch when in rtl mode:
      theme/base/layout/frontpage.php
      theme/base/layout/general.php

      Right align course completion checkbox to the right side of the activities and resources, when in RTL mode
      theme/base/style/course.css

      Right align DOCK bar, when in RTL mode
      theme/base/style/dock.css

      First-name and Last-name Initials (filter) bar (on every users list) letters spacing (usability issue, NOT RTL issue)
      theme/base/style/core.css

      I'll be committing more fixes in the comments to this issue since they are relevant to this patch but are more specific to the LESSON and GLOSSARY modules. (I made the changes in the respective Activity style.css file, but they could be added to the theme/base/styles as well)

        Issue Links

          Activity

          Hide
          Nadav Kavalerchik added a comment -

          MDL-32479 - RTL Theme fixes for Moodle 2 (theme/base)
          Right align content (when viewing a Lesson activity in RTL mode)
          https://github.com/nadavkav/moodle/commit/46e0a6335ba3cbb2484d457ea0ec0c57f6b8f5a2

          MDL-32479 - RTL Theme fixes for Moodle 2 (theme/base)
          Right align item's content (when viewing a Glossary activity in RTL mode)
          https://github.com/nadavkav/moodle/commit/67de8bcb47c48f71cd5e632ef0cdbd27f1789e69

          Show
          Nadav Kavalerchik added a comment - MDL-32479 - RTL Theme fixes for Moodle 2 (theme/base) Right align content (when viewing a Lesson activity in RTL mode) https://github.com/nadavkav/moodle/commit/46e0a6335ba3cbb2484d457ea0ec0c57f6b8f5a2 MDL-32479 - RTL Theme fixes for Moodle 2 (theme/base) Right align item's content (when viewing a Glossary activity in RTL mode) https://github.com/nadavkav/moodle/commit/67de8bcb47c48f71cd5e632ef0cdbd27f1789e69
          Hide
          Mary Evans added a comment -

          Nadav, if you are changing mods like Glossary & Lesson then these do not need to go into BASE theme.

          Please make a NEW sub-task to deal with mods then this will keep thing separate and easier to manage.

          I'll look at this and let you know if I spot any problems.

          By the way, do you have permission to set tracker issues for Integration Review?

          Cheers
          Mary

          Show
          Mary Evans added a comment - Nadav, if you are changing mods like Glossary & Lesson then these do not need to go into BASE theme. Please make a NEW sub-task to deal with mods then this will keep thing separate and easier to manage. I'll look at this and let you know if I spot any problems. By the way, do you have permission to set tracker issues for Integration Review? Cheers Mary
          Hide
          Nadav Kavalerchik added a comment -

          I have made two new sub tasks.
          And I have the right permissions to assign someone to do Integration review (not sure exactly what does mean and whom i can assign it to)
          How is it differ from Peer review?

          Show
          Nadav Kavalerchik added a comment - I have made two new sub tasks. And I have the right permissions to assign someone to do Integration review (not sure exactly what does mean and whom i can assign it to) How is it differ from Peer review?
          Hide
          Mary Evans added a comment -

          Hi,
          Peer Review is so that you can ask someone to look at and test your changes before submitting them for 'Integration'.

          'Integration Review' is that part of the process that gets all the 'commits' into Moodle. But first is has to be agreed by the Integrator, usually Sam or Eloy or whoever happens to be in the 'driving seat' at the time, on condition there are no errors in the code and that it passes any test that are carried out on it. It is perhaps more complicated that my idea of how it all works, but this part can be either the 'life' or 'death' of your commit...a bit scary!

          Show
          Mary Evans added a comment - Hi, Peer Review is so that you can ask someone to look at and test your changes before submitting them for 'Integration'. 'Integration Review' is that part of the process that gets all the 'commits' into Moodle. But first is has to be agreed by the Integrator, usually Sam or Eloy or whoever happens to be in the 'driving seat' at the time, on condition there are no errors in the code and that it passes any test that are carried out on it. It is perhaps more complicated that my idea of how it all works, but this part can be either the 'life' or 'death' of your commit...a bit scary!
          Hide
          Mary Evans added a comment - - edited

          Also worth knowing...Patrick Malley as Theme Manager is automatically assigned to any tracker issue whose component = 'Themes'. So when creating these 'sub-tasks' you can assign them to yourself. And then ask me or whoever you want, to Peer Review them and when you are happy for it to be submitted for integration Review, if you see the setting then "Submit for Integration', go for it!.

          Hope this helps?

          Mary

          Show
          Mary Evans added a comment - - edited Also worth knowing...Patrick Malley as Theme Manager is automatically assigned to any tracker issue whose component = 'Themes'. So when creating these 'sub-tasks' you can assign them to yourself. And then ask me or whoever you want, to Peer Review them and when you are happy for it to be submitted for integration Review, if you see the setting then "Submit for Integration', go for it!. Hope this helps? Mary
          Show
          Nadav Kavalerchik added a comment - more updates: https://github.com/nadavkav/moodle/commit/62d970145089e1df142ca229d51fa3eb61953886
          Hide
          Mary Evans added a comment -

          For some reason I cant make this a sub-task as you requested.

          Show
          Mary Evans added a comment - For some reason I cant make this a sub-task as you requested.
          Hide
          Nadav Kavalerchik added a comment -

          I have changed it into a Task and linked it to MDL-32821
          I have no idea, how to make it a sub-task.

          Show
          Nadav Kavalerchik added a comment - I have changed it into a Task and linked it to MDL-32821 I have no idea, how to make it a sub-task.
          Hide
          Mary Evans added a comment -

          I've just realised the reason I could not convert this to a sub-task, it was becasue it was a parent for two sub-sasks already. I have fixed this now and all are part of MDL-32821

          Thanks
          Mary

          Show
          Mary Evans added a comment - I've just realised the reason I could not convert this to a sub-task, it was becasue it was a parent for two sub-sasks already. I have fixed this now and all are part of MDL-32821 Thanks Mary
          Hide
          Mary Evans added a comment -

          Just about to create branch MDL-32479 in GIT to add the RTL fixes that Nadav has been working on these past few weeks.

          Show
          Mary Evans added a comment - Just about to create branch MDL-32479 in GIT to add the RTL fixes that Nadav has been working on these past few weeks.
          Hide
          Mary Evans added a comment -

          All CREDIT goes to Nadav Kavalerchik for his work getting Moodle 2.3 uptospeed with RTL fixes. This is one of many commits being pushed to Moodle.

          Show
          Mary Evans added a comment - All CREDIT goes to Nadav Kavalerchik for his work getting Moodle 2.3 uptospeed with RTL fixes. This is one of many commits being pushed to Moodle.
          Hide
          Mary Evans added a comment -

          @Nadav

          I removed the filepicker code from ../theme/base/style/core.css as there is a css file called filemanager.css which was recently added to Base theme, and carries a whole bunch of code for .file-picker.

          However, if the .file-picker code you added still needs to go into Base theme, then I will add it to the filemanager.css

          I will add the .dir-rtl for the filepicker in a seperate commit this weekend.

          So can you let me know ASAP?

          Thanks

          Show
          Mary Evans added a comment - @Nadav I removed the filepicker code from ../theme/base/style/core.css as there is a css file called filemanager.css which was recently added to Base theme, and carries a whole bunch of code for .file-picker. However, if the .file-picker code you added still needs to go into Base theme, then I will add it to the filemanager.css I will add the .dir-rtl for the filepicker in a seperate commit this weekend. So can you let me know ASAP? Thanks
          Hide
          Nadav Kavalerchik added a comment -

          Yes, I have noticed that after I made some file-picker fixes.

          I am almost finished fixing the file-picker and the new YUI3 Activity chooser.

          I have also just now pulled your github repo and checkout MDL-32479 locally.
          Reviewing the last 3 commits, It seems you have copied most of the stuff. Please let me know when you are all done, so i can add all the rest which i am still working on, over here.

          btw, I found the RTL CSS rules more easy to manage when i duplicate a CSS rule, I am about to fix, immediately in the same file and add the .dir-rtl fix directly after the original CSS rule. Instead of creating a separate rtl.css file for all the RTL rules.
          What are your opinions on that?

          Show
          Nadav Kavalerchik added a comment - Yes, I have noticed that after I made some file-picker fixes. I am almost finished fixing the file-picker and the new YUI3 Activity chooser. I have also just now pulled your github repo and checkout MDL-32479 locally. Reviewing the last 3 commits, It seems you have copied most of the stuff. Please let me know when you are all done, so i can add all the rest which i am still working on, over here. btw, I found the RTL CSS rules more easy to manage when i duplicate a CSS rule, I am about to fix, immediately in the same file and add the .dir-rtl fix directly after the original CSS rule. Instead of creating a separate rtl.css file for all the RTL rules. What are your opinions on that?
          Hide
          Mary Evans added a comment - - edited

          These commits are in line for integration which will be starting tomorrow evening in UK (which is Monday morning in Australia). If it gets picked up and tested as OK then this will be in Moodle 2.3 by Wednesday/Thursday, but it all depends on Moodle HQ.

          Adding RTL css direct after the original is the preferred way, so carry on!

          Show
          Mary Evans added a comment - - edited These commits are in line for integration which will be starting tomorrow evening in UK (which is Monday morning in Australia). If it gets picked up and tested as OK then this will be in Moodle 2.3 by Wednesday/Thursday, but it all depends on Moodle HQ. Adding RTL css direct after the original is the preferred way, so carry on!
          Hide
          Dan Poltawski added a comment -

          Sam, i'm assigning this to you to integrate because I think you have the best knowledge of this one.

          Show
          Dan Poltawski added a comment - Sam, i'm assigning this to you to integrate because I think you have the best knowledge of this one.
          Hide
          Marina Glancy added a comment -

          Hi Mary, I saw your changes to filemanager.css
          Maybe you can link Barbara to some guidelines how css styles are supposed to be in Moodle so she does it correctly herself.

          Show
          Marina Glancy added a comment - Hi Mary, I saw your changes to filemanager.css Maybe you can link Barbara to some guidelines how css styles are supposed to be in Moodle so she does it correctly herself.
          Hide
          Mary Evans added a comment -

          CSS has it's own standards which Barbara will be aware of, however I am not aware that there are any guidelines for CSS for Moodle, well not official ones at least, although I did read that someone was thinking about one.

          Show
          Mary Evans added a comment - CSS has it's own standards which Barbara will be aware of, however I am not aware that there are any guidelines for CSS for Moodle, well not official ones at least, although I did read that someone was thinking about one.
          Hide
          Mary Evans added a comment -

          I've cherry-picked the main RTL fixes for BASE theme from my previous attempt at this. I'll redo the filemanager.css as a subtask to keep it separate from this branch.

          Show
          Mary Evans added a comment - I've cherry-picked the main RTL fixes for BASE theme from my previous attempt at this. I'll redo the filemanager.css as a subtask to keep it separate from this branch.
          Hide
          Mary Evans added a comment -

          @Aparup

          I've just added you as a watcher to keep you in the loop.

          Show
          Mary Evans added a comment - @Aparup I've just added you as a watcher to keep you in the loop.
          Hide
          Aparup Banerjee added a comment -

          Mary, i've integrated some commits from here. (when this issue was closed at one point.)
          commits from this issue integrated are reflected here : https://github.com/nebgor/moodle/compare/mistress...MDL-30337_no_risk

          Show
          Aparup Banerjee added a comment - Mary, i've integrated some commits from here. (when this issue was closed at one point.) commits from this issue integrated are reflected here : https://github.com/nebgor/moodle/compare/mistress...MDL-30337_no_risk
          Hide
          Mary Evans added a comment -

          In that case this is not needed.

          Can you close it if that is the case?

          Many thanks.

          Show
          Mary Evans added a comment - In that case this is not needed. Can you close it if that is the case? Many thanks.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Looks like the two of you are on top of this, however I had a look through just to see what was left.
          It appears there may be a couple of things in here that are still wanted.
          The following are the things I saw changed when merging this patch and my thoughts on each below that.

          1. Changed CSS class from has_custom_menu to has-custom-menu in base theme layouts.
          2. blocks.css: Minor change to `.block .header .block_action`. margin-top changed from 0px => 2px
          3. core.css: Changed `.mform .fitem fieldset.felement` margin-left from 15% => 0
          4. core.css: Change #yui-gen4/6 => filepicker in selector
          5. core.css: Addition of selector `.initialbar a` on line 808.
          6. core.css: `.dir-rtl td[align="left"]` and friend introduced to reverse direction of table cells with set align values.
          7. core.css: Addition of selector `yui-module-debug`
          8. course.css: Addition of dir-rtl selectors for course category tree
          9. course.css: Changed span.editinstructions to dir-rtl only selector.

          Looking at and thinking about those points.

          1. I am happy for this change to be made but I think it should be done in 2.4, it's not essential and standard theme still has styles for has_custom_menu (it doesn't have layouts and uses bases so would be a regression). It should be a separate issue, and I see we have a mix of has_custom_menu and has-custom-menu in the themes. I think perhaps we should standardise that (all part of that one issue).
          2. Fine
          3. This looks like an interesting change, can you explain what its purpose is and where I can see its effect.
          4. Fine
          5. This style is already defined on line 981... but perhaps from the earlier merge?
          6. I appreciate the idea behind this, and it is a smart solution, but really we should be identifying those locations in core and fixing them by either using classes, or checking language direction in before setting the style. Otherwise if someone is already checking the direction things will be unwantingly reversed.
          7. This is no longer needed. It was removed recently as the module debug information is no longer included on every page. In order to see that information the user now sets a config variable.
          8. Fine
          9. This change looks a bit suspect, perhaps something went wrong in the merge.

          Sounds like this issue may be coming to an end but before it does would you mind having a look through that list Mary and seeing if there is anything you think needs to land before 2.3. If there is then we can look to get it in and write test instructions for it explicitly.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Looks like the two of you are on top of this, however I had a look through just to see what was left. It appears there may be a couple of things in here that are still wanted. The following are the things I saw changed when merging this patch and my thoughts on each below that. Changed CSS class from has_custom_menu to has-custom-menu in base theme layouts. blocks.css: Minor change to ` .block .header .block_action `. margin-top changed from 0px => 2px core.css: Changed ` .mform .fitem fieldset.felement ` margin-left from 15% => 0 core.css: Change #yui-gen4/6 => filepicker in selector core.css: Addition of selector ` .initialbar a ` on line 808. core.css: ` .dir-rtl td [align="left"] ` and friend introduced to reverse direction of table cells with set align values. core.css: Addition of selector ` yui-module-debug ` course.css: Addition of dir-rtl selectors for course category tree course.css: Changed span.editinstructions to dir-rtl only selector. Looking at and thinking about those points. I am happy for this change to be made but I think it should be done in 2.4, it's not essential and standard theme still has styles for has_custom_menu (it doesn't have layouts and uses bases so would be a regression). It should be a separate issue, and I see we have a mix of has_custom_menu and has-custom-menu in the themes. I think perhaps we should standardise that (all part of that one issue). Fine This looks like an interesting change, can you explain what its purpose is and where I can see its effect. Fine This style is already defined on line 981... but perhaps from the earlier merge? I appreciate the idea behind this, and it is a smart solution, but really we should be identifying those locations in core and fixing them by either using classes, or checking language direction in before setting the style. Otherwise if someone is already checking the direction things will be unwantingly reversed. This is no longer needed. It was removed recently as the module debug information is no longer included on every page. In order to see that information the user now sets a config variable. Fine This change looks a bit suspect, perhaps something went wrong in the merge. Sounds like this issue may be coming to an end but before it does would you mind having a look through that list Mary and seeing if there is anything you think needs to land before 2.3. If there is then we can look to get it in and write test instructions for it explicitly. Cheers Sam
          Hide
          Mary Evans added a comment -

          Hi Sam,
          Thanks for this.

          As long as all the DIR-RTL changes from these commits have made their way into 2.3
          I think the stragglers can wait.

          I'll check with Nadav to be on the safe side.

          Show
          Mary Evans added a comment - Hi Sam, Thanks for this. As long as all the DIR-RTL changes from these commits have made their way into 2.3 I think the stragglers can wait. I'll check with Nadav to be on the safe side.
          Hide
          Mary Evans added a comment -

          @Nadav

          Is there anything urgent which you feel needs to go into Moodle 2.3 base/style/core.css from the list Sam commented on?

          Show
          Mary Evans added a comment - @Nadav Is there anything urgent which you feel needs to go into Moodle 2.3 base/style/core.css from the list Sam commented on?
          Hide
          Aparup Banerjee added a comment -

          ah point (5) above is my bad, i didn't spot it in the vicinity so

          Show
          Aparup Banerjee added a comment - ah point (5) above is my bad, i didn't spot it in the vicinity so
          Hide
          Nadav Kavalerchik added a comment -

          @Sam

          3. I wish i could remember too (to many commits too long ago)
          If in doubt, leave it out.

          6. As far as i can remember, it is related to the modal dialogs (Insert Link/Image/Multimedia) that popup off the TinyMCE editor which are hard coded LTR. A while ago, I was fixing each YUI3 version, but stopped since it was not an elegant solution.
          I guess, We should try to override it from the theme's level or have it fix by the YUI developers. ( which did not responded to my posts on the developers forum )

          If you sill have time to fix RTL issues...
          Just pulled a new 2.3dev version from git and saw that (for example: theme/magazine):
          1. Custom menu's menus are not right aligned and the sub menus open in LTR mode (to the right of the menu)
          2. Block columns do not switch sides when in RTL mode
          3. TinyMCE Insert modal dialogs are all justified the the left, when in RTL mode
          4. Breadcrumb is left justified

          Anyways... I will start going through all the needed RTL changes to 2.4dev in about 2-3 weeks.

          Show
          Nadav Kavalerchik added a comment - @Sam 3. I wish i could remember too (to many commits too long ago) If in doubt, leave it out. 6. As far as i can remember, it is related to the modal dialogs (Insert Link/Image/Multimedia) that popup off the TinyMCE editor which are hard coded LTR. A while ago, I was fixing each YUI3 version, but stopped since it was not an elegant solution. I guess, We should try to override it from the theme's level or have it fix by the YUI developers. ( which did not responded to my posts on the developers forum ) If you sill have time to fix RTL issues... Just pulled a new 2.3dev version from git and saw that (for example: theme/magazine): 1. Custom menu's menus are not right aligned and the sub menus open in LTR mode (to the right of the menu) 2. Block columns do not switch sides when in RTL mode 3. TinyMCE Insert modal dialogs are all justified the the left, when in RTL mode 4. Breadcrumb is left justified Anyways... I will start going through all the needed RTL changes to 2.4dev in about 2-3 weeks.
          Hide
          Nadav Kavalerchik added a comment - - edited

          One more MAJOR issue:
          None of the resources and activities are visible in the new modal "Add an activity or resource" dialog
          quick fix:

          .dir-rtl .choosercontainer #chooseform .instruction, 
          .dir-rtl .jsenabled .choosercontainer #chooseform .typesummary {
          right: 18.5em;
          left: 0;
          }
          
          Show
          Nadav Kavalerchik added a comment - - edited One more MAJOR issue: None of the resources and activities are visible in the new modal "Add an activity or resource" dialog quick fix: .dir-rtl .choosercontainer #chooseform .instruction, .dir-rtl .jsenabled .choosercontainer #chooseform .typesummary { right: 18.5em; left: 0; }
          Hide
          Sam Hemelryk added a comment -

          Ok thanks guys, I've integrated a partial fix now.

          Points 2,4,5, and 8 have been integrated.

          Points 1, 3, and 9 should be separate issues if we still want to look at them.
          Point 6 individual areas in core need to be identified, issues created for each, and then fixed.
          Point 7 is no longer needed.

          Thanks everyone for the effort.
          Nadav I see someone has already fixed the mod chooser in another issue (yay)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok thanks guys, I've integrated a partial fix now. Points 2,4,5, and 8 have been integrated. Points 1, 3, and 9 should be separate issues if we still want to look at them. Point 6 individual areas in core need to be identified, issues created for each, and then fixed. Point 7 is no longer needed. Thanks everyone for the effort. Nadav I see someone has already fixed the mod chooser in another issue (yay) Cheers Sam
          Hide
          Adrian Greeve added a comment -

          I discovered a problem with Arabic and the calendar block. I would say that the names of the days of the week are pushing the table width past the size allowed for the block. I included a screen shot of the formatting of the calendar block. Besides this all of the other formatting looks good.

          Show
          Adrian Greeve added a comment - I discovered a problem with Arabic and the calendar block. I would say that the names of the days of the week are pushing the table width past the size allowed for the block. I included a screen shot of the formatting of the calendar block. Besides this all of the other formatting looks good.
          Hide
          Mary Evans added a comment -

          I'll create a new sub-task for the calendar if the rest of these changes can be integrated?

          Show
          Mary Evans added a comment - I'll create a new sub-task for the calendar if the rest of these changes can be integrated?
          Hide
          Mary Evans added a comment -

          It looks like Arabic uses full names for the days of the week, whereas Hebrew abbreviates to just a single letter which works better. So I presume this can be fixed in /lang/ar/ language strings? I don't know enough about such things, indeed, the days of the week may come from another source which needs looking into.

          Can you suggest a likely person that could look into this?

          Show
          Mary Evans added a comment - It looks like Arabic uses full names for the days of the week, whereas Hebrew abbreviates to just a single letter which works better. So I presume this can be fixed in /lang/ar/ language strings? I don't know enough about such things, indeed, the days of the week may come from another source which needs looking into. Can you suggest a likely person that could look into this?
          Hide
          Mary Evans added a comment -

          Attached image of calendar in Hebrew

          Show
          Mary Evans added a comment - Attached image of calendar in Hebrew
          Hide
          Nadav Kavalerchik added a comment - - edited

          @Adrian
          It's not a CSS issue. ( as far as i can tell and also just asked around, my fellow arabic speaking friends )
          It's a Translation issue.
          The names of the days in the week should be abbreviated to 1 to 3 letters at most.
          Than, it will fit. (like in other languages)

          Checkout: http://www.deepgraysea.com/images/uistyleguidecalarabic.gif

          Show
          Nadav Kavalerchik added a comment - - edited @Adrian It's not a CSS issue. ( as far as i can tell and also just asked around, my fellow arabic speaking friends ) It's a Translation issue. The names of the days in the week should be abbreviated to 1 to 3 letters at most. Than, it will fit. (like in other languages) Checkout: http://www.deepgraysea.com/images/uistyleguidecalarabic.gif
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: