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
    • Rank:
      45133

      Description

      follow up for MDL-36048

      Themes to be altered:

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

      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: