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 Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5.1
    • Component/s: Lesson, Themes
    • Labels:
    • Rank:
      50510

      Description

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

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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 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 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 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
        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
        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
        Rossiani Wijaya added a comment -

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

        Show
        Rossiani Wijaya added a comment - +1 for Damyon's suggestion and updated patch accordingly.
        Hide
        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
        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
        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
        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
        Rossiani Wijaya added a comment -

        Resubmitting for integration review.

        Show
        Rossiani Wijaya added a comment - Resubmitting for integration review.
        Hide
        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
        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
        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
        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
        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
        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
        Dan Poltawski added a comment -

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

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

        Oops, I thought i'd pushed this.

        Thanks Rosie

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

        Works as described. Thanks Rossi

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

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

        Closing as fixed!

        Show
        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: