Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4.1
    • Component/s: Course, Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Install course format 'testheaders' from attachment. It is basically the copy of topics format but with displaying test course headers/footers
      2. Create one course in 'topics' (or any other existing) format and one in 'testheaders' format
      3. Open 4 tabs - the course view page, and any page with 'report' layout inside the course, for each of those courses
      4. In 5th tab keep changing the theme for the site and refresh the previous 4 tabs, make sure that page layout looks ok, html is not broken for existing format and headers and footers appear in place in testheaders format.(Test the themes listed in description)
      Show
      Install course format 'testheaders' from attachment. It is basically the copy of topics format but with displaying test course headers/footers Create one course in 'topics' (or any other existing) format and one in 'testheaders' format Open 4 tabs - the course view page, and any page with 'report' layout inside the course, for each of those courses In 5th tab keep changing the theme for the site and refresh the previous 4 tabs, make sure that page layout looks ok, html is not broken for existing format and headers and footers appear in place in testheaders format.(Test the themes listed in description)
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-36339-master

      Description

      follow up for MDL-36048

      Themes to be altered:

      formal_white
      formfactor
      fusion
      leatherbound
      magazine
      mymobile
      nimble
      nonzero
      overlay
      sky_high
      splash
      standardold

        Gliffy Diagrams

        1. testheaders.tar.gz
          4 kB
          Marina Glancy
        1. EnrolPageFormalWhite.png
          30 kB
        2. Standard (legacy).png
          131 kB

          Issue Links

            Activity

            Hide
            Aparup Banerjee added a comment -

            @Mary: perhaps useful to have this go past your eyes too

            Show
            Aparup Banerjee added a comment - @Mary: perhaps useful to have this go past your eyes too
            Hide
            Mary Evans added a comment -

            Why is this still hanging around I thought this had all gone through, and fixed?

            Show
            Mary Evans added a comment - Why is this still hanging around I thought this had all gone through, and fixed?
            Hide
            Aparup Banerjee added a comment -

            oh, it does seem some of these options got through via MDL-36048, not sure why these were done separately though.

            Show
            Aparup Banerjee added a comment - oh, it does seem some of these options got through via MDL-36048 , not sure why these were done separately though.
            Hide
            Marina Glancy added a comment - - edited

            because I was running out of time before the code freeze and wanted to push the changes to backend and basic themes first. Dan confirmed that it is ok to split the issue and push the rest later, after code freeze

            Show
            Marina Glancy added a comment - - edited because I was running out of time before the code freeze and wanted to push the changes to backend and basic themes first. Dan confirmed that it is ok to split the issue and push the rest later, after code freeze
            Hide
            Sam Hemelryk added a comment -

            Hi Marina,

            Just checking but these changes should land to both 24 and master correct?

            Show
            Sam Hemelryk added a comment - Hi Marina, Just checking but these changes should land to both 24 and master correct?
            Hide
            Marina Glancy added a comment -

            yes please

            Show
            Marina Glancy added a comment - yes please
            Hide
            Marina Glancy added a comment -

            rebased

            Show
            Marina Glancy added a comment - rebased
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Side warn: just note the part 1 issue still has the dev_docs_required label, so I guess that change in themes has not been documented.

            Show
            Eloy Lafuente (stronk7) added a comment - Side warn: just note the part 1 issue still has the dev_docs_required label, so I guess that change in themes has not been documented.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Side note: I'm going to integrate this but it hurts my eyes a bit to see the same block of code repeated and repeated over all themes. Wouldn't be better to have it encapsulated into a function?

            https://github.com/marinaglancy/moodle/compare/master...wip-MDL-36339-master#L29R37

            Show
            Eloy Lafuente (stronk7) added a comment - Side note: I'm going to integrate this but it hurts my eyes a bit to see the same block of code repeated and repeated over all themes. Wouldn't be better to have it encapsulated into a function? https://github.com/marinaglancy/moodle/compare/master...wip-MDL-36339-master#L29R37
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (24 and master), thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (24 and master), thanks!
            Hide
            Marina Glancy added a comment -

            Hi Eloy,
            not really because those configuration options may be called differently in other themes. The options are just called exactly the same in all core themes. There is other code repeated in multiple themes as well, i.e.

            $custommenu = $OUTPUT->custom_menu();
            $hascustommenu = (empty($PAGE->layout_options['nocustommenu']) && !empty($custommenu));
            

            Show
            Marina Glancy added a comment - Hi Eloy, not really because those configuration options may be called differently in other themes. The options are just called exactly the same in all core themes. There is other code repeated in multiple themes as well, i.e. $custommenu = $OUTPUT->custom_menu(); $hascustommenu = (empty($PAGE->layout_options['nocustommenu']) && !empty($custommenu));
            Hide
            Andrew Davis added a comment -

            Attaching screenshots of two visual oddities. I'm not sure if they are due to this fix.

            Firstly, the bottom enrol button on the course enrol users page is over the bottom course content area in every theme.

            Secondly, the blocks in Standard (legacy) take up the full page width. This occurs in both a course in topic format and the testheaders format.

            Show
            Andrew Davis added a comment - Attaching screenshots of two visual oddities. I'm not sure if they are due to this fix. Firstly, the bottom enrol button on the course enrol users page is over the bottom course content area in every theme. Secondly, the blocks in Standard (legacy) take up the full page width. This occurs in both a course in topic format and the testheaders format.
            Hide
            Andrew Davis added a comment -

            I'm going to be offline for a few hours. Feel free to fix these issues here, to pass this issue and to split them out into a new MDL or to pass this issue and to deem them already covered by an existing MDL, in my absence

            Show
            Andrew Davis added a comment - I'm going to be offline for a few hours. Feel free to fix these issues here, to pass this issue and to split them out into a new MDL or to pass this issue and to deem them already covered by an existing MDL, in my absence
            Hide
            Marina Glancy added a comment -

            Hi Andrew,
            re first screenshot: this may not be directly related, it is just html/css issue with particular page. It just never came up before because there never was any content after the table. You can create an issue about it. The solution is to insert a line before https://github.com/moodle/moodle/blob/master/enrol/renderer.php#L93 :

            $content .= html_writer::tag('div', '', array('class' => 'clearfix'));
            

            re second screenshot: this is exactly the same in 2.3 without any course format changes, so it's unrelated

            Show
            Marina Glancy added a comment - Hi Andrew, re first screenshot: this may not be directly related, it is just html/css issue with particular page. It just never came up before because there never was any content after the table. You can create an issue about it. The solution is to insert a line before https://github.com/moodle/moodle/blob/master/enrol/renderer.php#L93 : $content .= html_writer::tag('div', '', array('class' => 'clearfix')); re second screenshot: this is exactly the same in 2.3 without any course format changes, so it's unrelated
            Hide
            Andrew Davis added a comment -

            I've raised MDL-37242. Passing.

            Show
            Andrew Davis added a comment - I've raised MDL-37242 . Passing.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Changes are now upstream, thanks for your collaboration!

            If you are going to have any celebration next days, enjoy with your gang, if not, too!

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Changes are now upstream, thanks for your collaboration! If you are going to have any celebration next days, enjoy with your gang, if not, too! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: