Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38856 META: Issues around Bootstrapbase and Clean theme
  3. MDL-39773

Missing a space between "Return to lesson" and "View grades" at the end of lesson page

    Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Lesson, Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Set theme to clean
      2. Create a lesson and add a question
      3. Attempt the lesson as student

      At the end of lesson page, make sure there are some spaces between "Return to lesson" and "View grades" links.

      1. Repeat the test in all themes to ensure the style looks good.
      Show
      Set theme to clean Create a lesson and add a question Attempt the lesson as student At the end of lesson page, make sure there are some spaces between "Return to lesson" and "View grades" links. Repeat the test in all themes to ensure the style looks good.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Add a space between "Return to lesson" and "View grades" at the end of lesson page

        Gliffy Diagrams

          Activity

          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for fixing this Rossie,

          I can see you have used similar solution as in standard theme (theme/standard/style/modules.css).
          I was wondering, if you should consider padding-right or selector rather then padding. Padding will add space for first link as well.
          Feel free to push it for integration if you feel otherwise.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Rossie, I can see you have used similar solution as in standard theme (theme/standard/style/modules.css). I was wondering, if you should consider padding-right or selector rather then padding. Padding will add space for first link as well. Feel free to push it for integration if you feel otherwise.
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Raj,

          Thank you for reviewing.

          The reason I used padding instead of padding-right is to make it compatible with RTL.

          I'm not sure whether padding-right will automatically converted to padding-left if we use RTL language.

          Will report my finding later.

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Raj, Thank you for reviewing. The reason I used padding instead of padding-right is to make it compatible with RTL. I'm not sure whether padding-right will automatically converted to padding-left if we use RTL language. Will report my finding later.
          Hide
          rwijaya Rossiani Wijaya added a comment -

          I had a discussion with Raj regarding the RTL format and decided that it would be better to create new css class for RTL.

          Patch updated.

          Sending this for integration review.

          Show
          rwijaya Rossiani Wijaya added a comment - I had a discussion with Raj regarding the RTL format and decided that it would be better to create new css class for RTL. Patch updated. Sending this for integration review.
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Guys,

          Well, i'm afraid that I don't really agree with this solution:

          1. We should be consistent between base (defined in standaard/canvas) and boostrapbase, surely? So if this is the fix for bootstrapbase, it should be the fix for base too?
          2. It's called .centerpadded - that implies the same padding on both sides to make it centered. I think its really confusing to introduce this as not quite centred at all.

          Really I think you should've gone for the blanket padded solution, to be consistent with base. But if you want to be more elegant about it then I think you should name it better and make it consistent between base and bootstrap base.

          Show
          poltawski Dan Poltawski added a comment - Hi Guys, Well, i'm afraid that I don't really agree with this solution: We should be consistent between base (defined in standaard/canvas) and boostrapbase, surely? So if this is the fix for bootstrapbase, it should be the fix for base too? It's called .centerpadded - that implies the same padding on both sides to make it centered. I think its really confusing to introduce this as not quite centred at all. Really I think you should've gone for the blanket padded solution, to be consistent with base. But if you want to be more elegant about it then I think you should name it better and make it consistent between base and bootstrap base.
          Hide
          damyon Damyon Wiese added a comment -

          Some comments:

          centerpadded is a terrible name for a class - but it is existing so oh well.

          This is currently styled in "standard" and "canvas" themes - so for just a fix it would be appropriate to move those style rules to base and bootstrapbase as is (Currently sets both padding and "text-align: center"). Lets just stick to that and not try and only put padding on one side (to match the name of the class and the existing css). Don't forget to remove this from standard and canvas themes as well.

          For future development - we should be thinking of this as a navigational element which contains a list of links. It may be displayed in may ways, such as a menu, list of links, url select - whatever and not be specific to lesson. I dont think we are ready to start making changes like that yet though - so +1 for quick fix.

          Show
          damyon Damyon Wiese added a comment - Some comments: centerpadded is a terrible name for a class - but it is existing so oh well. This is currently styled in "standard" and "canvas" themes - so for just a fix it would be appropriate to move those style rules to base and bootstrapbase as is (Currently sets both padding and "text-align: center"). Lets just stick to that and not try and only put padding on one side (to match the name of the class and the existing css). Don't forget to remove this from standard and canvas themes as well. For future development - we should be thinking of this as a navigational element which contains a list of links. It may be displayed in may ways, such as a menu, list of links, url select - whatever and not be specific to lesson. I dont think we are ready to start making changes like that yet though - so +1 for quick fix.
          Hide
          cibot CiBoT added a comment -

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

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

          I didn't realise the class name (.centerpadded), so yes +1 to have padding rather then padding-right.
          Saying so IMHO, we should consider having a different class to avoid left padding for first link. Consider having another issue to fix it.

          Show
          rajeshtaneja Rajesh Taneja added a comment - I didn't realise the class name (.centerpadded), so yes +1 to have padding rather then padding-right. Saying so IMHO, we should consider having a different class to avoid left padding for first link. Consider having another issue to fix it.
          Hide
          rwijaya Rossiani Wijaya added a comment -

          +1 for Damyon's suggestion and updated patch accordingly.

          Show
          rwijaya Rossiani Wijaya added a comment - +1 for Damyon's suggestion and updated patch accordingly.
          Hide
          lazydaisy Mary Evans added a comment -

          Just seen this, did someone add me in here or is it because I am watching themes?

          Could this particular problem not be fixed at source rather than the CSS quick fix idea?

          If a space is needed then surely wherever that page is generated should make provision for the way is is displayed, shouldn't it?

          Show
          lazydaisy Mary Evans added a comment - Just seen this, did someone add me in here or is it because I am watching themes? Could this particular problem not be fixed at source rather than the CSS quick fix idea? If a space is needed then surely wherever that page is generated should make provision for the way is is displayed, shouldn't it?
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Mary,

          I think you were automatically added as a watcher because you were either the component lead for themes or watching themes component.

          Currently,the problem only occurs in bootstrap and since we are in the process of improving the frontend of Moodle, this issue might be addressed as part of the improvement. So as for now, it would be the best to have a quick fix for the issue.

          I created a patch to add .centerpadded styling to bootstrapbase and relocate .centerpadded styling from standard to base themes. The purpose of relocation is to have some consistency between base and bootstrapebase styling.

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Mary, I think you were automatically added as a watcher because you were either the component lead for themes or watching themes component. Currently,the problem only occurs in bootstrap and since we are in the process of improving the frontend of Moodle, this issue might be addressed as part of the improvement. So as for now, it would be the best to have a quick fix for the issue. I created a patch to add .centerpadded styling to bootstrapbase and relocate .centerpadded styling from standard to base themes. The purpose of relocation is to have some consistency between base and bootstrapebase styling.
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Resubmitting for integration review.

          Show
          rwijaya Rossiani Wijaya added a comment - Resubmitting for integration review.
          Hide
          lazydaisy Mary Evans added a comment - - edited

          What was wrong with using the same CSS as is set for those buttons in base theme?

          .branchbuttoncontainer input {
              margin: 10px;
          }
          .branchbuttoncontainer.horizontal {
              text-align: center;
          

          Show
          lazydaisy Mary Evans added a comment - - edited What was wrong with using the same CSS as is set for those buttons in base theme? .branchbuttoncontainer input { margin: 10px; } .branchbuttoncontainer.horizontal { text-align: center;
          Hide
          poltawski Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          poltawski Dan Poltawski added a comment -

          Hi Rosie,

          Looks good to me - could you just expand the testing instructions to cover the other themes now?

          Show
          poltawski Dan Poltawski added a comment - Hi Rosie, Looks good to me - could you just expand the testing instructions to cover the other themes now?
          Hide
          poltawski Dan Poltawski added a comment -

          Rosie is away, so I have put a bit of a blanket statement in.

          Show
          poltawski Dan Poltawski added a comment - Rosie is away, so I have put a bit of a blanket statement in.
          Hide
          poltawski Dan Poltawski added a comment -

          Oops, I thought i'd pushed this.

          Thanks Rosie

          Show
          poltawski Dan Poltawski added a comment - Oops, I thought i'd pushed this. Thanks Rosie
          Hide
          phalacee Jason Fowler added a comment -

          Works as described. Thanks Rossi

          Show
          phalacee Jason Fowler added a comment - Works as described. Thanks Rossi
          Hide
          marina Marina Glancy added a comment -

          Thanks for your awesome work! This has now become a part of Moodle.

          Closing as fixed!

          Show
          marina Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                8/Jul/13