Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-42184

Bootstrap themes not hide navigation bar scorm activities

    Details

    • Testing Instructions:
      Hide
      1. Use clean theme
      2. Add a scrom activity (some can be found on moodle.net: http://moodle.net/mod/data/view.php?id=3)
      3. In the settings setup the activity to be displayed in a new window (popup)
      4. Go to the activity, display it, a popup should be opened. You should not see the navbar/header.
      Show
      Use clean theme Add a scrom activity (some can be found on moodle.net: http://moodle.net/mod/data/view.php?id=3 ) In the settings setup the activity to be displayed in a new window (popup) Go to the activity, display it, a popup should be opened. You should not see the navbar/header.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-42184-master-nomerge

      Description

      I have a small problem, when I open a scorm activity in a new window, I can not stop the navigation display.

      In any Bootstrap theme.

      I modified the config.php file

      'popup' => array(
      'file' => 'columns1.php',
      'regions' => array(),
      'options' => array('nofooter'=>true, 'nonavbar'=>true, 'noheader'=>true),

      But still not working.

      Thank you.

        Gliffy Diagrams

          Activity

          Hide
          lazydaisy Mary Evans added a comment - - edited

          I think that a popup.php file is needed for 'pop-up' layout which contains only the elements needed.

          Show
          lazydaisy Mary Evans added a comment - - edited I think that a popup.php file is needed for 'pop-up' layout which contains only the elements needed.
          Hide
          lazydaisy Mary Evans added a comment -

          Just added Dan Marsden as watcher and advisor.

          Show
          lazydaisy Mary Evans added a comment - Just added Dan Marsden as watcher and advisor.
          Hide
          danmarsden Dan Marsden added a comment -

          looks like base uses this for 'popup'
          array('nofooter'=>true, 'nonavbar'=>true, 'nocustommenu'=>true, 'nologininfo'=>true, 'nocourseheaderfooter'=>true),

          bootstrapbase uses this:
          array('nofooter'=>true, 'nonavbar'=>true),

          not sure on why that is... does the pop-up layout in bootstrap keep some of the header stuff intentionally because pop-ups work quite differently on mobile devices?

          Show
          danmarsden Dan Marsden added a comment - looks like base uses this for 'popup' array('nofooter'=>true, 'nonavbar'=>true, 'nocustommenu'=>true, 'nologininfo'=>true, 'nocourseheaderfooter'=>true), bootstrapbase uses this: array('nofooter'=>true, 'nonavbar'=>true), not sure on why that is... does the pop-up layout in bootstrap keep some of the header stuff intentionally because pop-ups work quite differently on mobile devices?
          Hide
          seryon Sergio López added a comment -

          thanks for the input, but I've tried every possible combination and still showing the navigation bar.

          Show
          seryon Sergio López added a comment - thanks for the input, but I've tried every possible combination and still showing the navigation bar.
          Hide
          lazydaisy Mary Evans added a comment - - edited

          I'm not sure either so adding some FRONTEND Team members to thrash this out as I am not well enough to work on this at the moment.

          Show
          lazydaisy Mary Evans added a comment - - edited I'm not sure either so adding some FRONTEND Team members to thrash this out as I am not well enough to work on this at the moment.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Personally I don't mind the header but I still removed the header from the popup to match standard behavior. I didn't backport it to 2.5 yet but I'll do it after peer-review.

          Show
          jerome Jérôme Mouneyrac added a comment - - edited Personally I don't mind the header but I still removed the header from the popup to match standard behavior. I didn't backport it to 2.5 yet but I'll do it after peer-review.
          Hide
          rwijaya Rossiani Wijaya added a comment -

          Hi Jerome,

          The patch looks great. Just very minor fix needed for core.less file, the indentation for the new css attribute.

          Also, some thought for the header configuration, would it be better to have the configuration on clean config file instead of bootstrap?

          Other than that, patch looks great.

          [y] Syntax
          [y] Whitespace
          [y] Output
          [-] Language
          [-] Databases
          [y] Testing (instructions and automated tests)
          [-] Security
          [-] Documentation
          [y] Git
          [-] Third party code
          [y] Sanity check

          Show
          rwijaya Rossiani Wijaya added a comment - Hi Jerome, The patch looks great. Just very minor fix needed for core.less file, the indentation for the new css attribute. Also, some thought for the header configuration, would it be better to have the configuration on clean config file instead of bootstrap? Other than that, patch looks great. [y] Syntax [y] Whitespace [y] Output [-] Language [-] Databases [y] Testing (instructions and automated tests) [-] Security [-] Documentation [y] Git [-] Third party code [y] Sanity check
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Thanks Rosie. As Clean extend Bootstrapbase, we should fix Bootstrapbase. Otherwise all extending Bootstrapbase theme would not have the fix and the point is to make the all behavior consistent.

          Show
          jerome Jérôme Mouneyrac added a comment - Thanks Rosie. As Clean extend Bootstrapbase, we should fix Bootstrapbase. Otherwise all extending Bootstrapbase theme would not have the fix and the point is to make the all behavior consistent.
          Hide
          marina Marina Glancy added a comment -

          as far as I know this is the approach that Sam Hemelryk does not prefer. Will be nice to hear his comments on it

          Show
          marina Marina Glancy added a comment - as far as I know this is the approach that Sam Hemelryk does not prefer. Will be nice to hear his comments on it
          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
          lazydaisy Mary Evans added a comment - - edited

          I am not sure this is the right way to fix this Jerome, as Bootstrap themes are very different from the norm, in that they have a top navbar (black or grey) which is used to hold the custommenu, lang menu and login/out. The navbar setting in the config.php controls the breadcrumb navbar and nothing else.

          If you don't want the custommenu in the top navbar then add the necessary code to stop the custommenu from displaying, but you cannot remove the top navbar as this is part of the design and responsiveness of the theme.

          The header contains a logo if one is set and links to the homepage, as does the top navbar, again if these are not required in SCORM pages then code them accordingly in bootstrapbase/renderers/core_renderer.php

          Show
          lazydaisy Mary Evans added a comment - - edited I am not sure this is the right way to fix this Jerome, as Bootstrap themes are very different from the norm, in that they have a top navbar (black or grey) which is used to hold the custommenu, lang menu and login/out. The navbar setting in the config.php controls the breadcrumb navbar and nothing else. If you don't want the custommenu in the top navbar then add the necessary code to stop the custommenu from displaying, but you cannot remove the top navbar as this is part of the design and responsiveness of the theme. The header contains a logo if one is set and links to the homepage, as does the top navbar, again if these are not required in SCORM pages then code them accordingly in bootstrapbase/renderers/core_renderer.php
          Hide
          samhemelryk Sam Hemelryk added a comment -

          I'm sending this back sorry Jerome.

          We worked to get all of the logic out of the bootstrapbase theme in favour of using layouts as they were really designed to be used.
          I see two preferable ways to solve this issue:

          1. Change the popup config in bootstrapbase/config.php so that it uses embedded.php rather than columns1.php
          2. Create a new layouts/popup.php, copy columns1.php content to that and then strip out anything we don't want to be in a popup.

          Personally I'd suggest reviewing uses of the popup layout an Moodle (perhaps in the standard theme to see what they are doing) and then in none of them require navigation which I suspect is the case go with option 1.
          If you go with option 2 you have to be mindful about what Mary has said, parts of the HTML are structural to the design and not just the functionality so removing entire blocks as you've done here needs consideration in respect to the overall design.

          Show
          samhemelryk Sam Hemelryk added a comment - I'm sending this back sorry Jerome. We worked to get all of the logic out of the bootstrapbase theme in favour of using layouts as they were really designed to be used. I see two preferable ways to solve this issue: Change the popup config in bootstrapbase/config.php so that it uses embedded.php rather than columns1.php Create a new layouts/popup.php, copy columns1.php content to that and then strip out anything we don't want to be in a popup. Personally I'd suggest reviewing uses of the popup layout an Moodle (perhaps in the standard theme to see what they are doing) and then in none of them require navigation which I suspect is the case go with option 1. If you go with option 2 you have to be mindful about what Mary has said, parts of the HTML are structural to the design and not just the functionality so removing entire blocks as you've done here needs consideration in respect to the overall design.
          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
          jerome Jérôme Mouneyrac added a comment -

          embedded.php don't have heading so we will have to add a popup.php.

          Show
          jerome Jérôme Mouneyrac added a comment - embedded.php don't have heading so we will have to add a popup.php.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          I disagree with the fact that a popup should have logo and link to home page. A window is not a popup for me if it contains a header. However, as I previously mentioned, I don't mind about having header or not in the popup, as I think that themer devs will anyway tweak it at their own will. It's what I would do. But as Standard doesn't have a visual header, so Bootstrap should not have it.

          Obviously it's just my opinion, and I'm not a theme expert. So if you really want the header tag (or nav tag), just explain with examples what I'm missing for not having header tag in the popup. I'm pushing a new version (which basically is just the proper coding of the previous fix).

          Note that I did try to put the header tag (without nav tag) but it added some space. So we could add a new class to remove the space or we could make the header display:none but all that feel wrong to me. I'm happy to make any change till I understand them

          Just to confirm I understand well the issue: in bootstrap, as it has been designed, the "nocustommenu, nocourseheaderfooter,..." are not used anymore.

          PS: I understand you may want the header for mobile, but maybe there should be no popup window in mobile, they should all be new tabs.

          resending to integration.

          Show
          jerome Jérôme Mouneyrac added a comment - I disagree with the fact that a popup should have logo and link to home page. A window is not a popup for me if it contains a header. However, as I previously mentioned, I don't mind about having header or not in the popup, as I think that themer devs will anyway tweak it at their own will. It's what I would do. But as Standard doesn't have a visual header, so Bootstrap should not have it. Obviously it's just my opinion, and I'm not a theme expert. So if you really want the header tag (or nav tag), just explain with examples what I'm missing for not having header tag in the popup. I'm pushing a new version (which basically is just the proper coding of the previous fix). Note that I did try to put the header tag (without nav tag) but it added some space. So we could add a new class to remove the space or we could make the header display:none but all that feel wrong to me. I'm happy to make any change till I understand them Just to confirm I understand well the issue: in bootstrap, as it has been designed, the "nocustommenu, nocourseheaderfooter,..." are not used anymore. PS: I understand you may want the header for mobile, but maybe there should be no popup window in mobile, they should all be new tabs. resending to integration.
          Hide
          lazydaisy Mary Evans added a comment -

          Hi Jerome, if 'navbar' (breadcrumb) and 'footer' are surplus to requirements in a 'popup' why add them in the first place? If I were you I would design the 'popup' page with no navbar and no footer. Then there is no need to add options.

          Show
          lazydaisy Mary Evans added a comment - Hi Jerome, if 'navbar' (breadcrumb) and 'footer' are surplus to requirements in a 'popup' why add them in the first place? If I were you I would design the 'popup' page with no navbar and no footer. Then there is no need to add options.
          Hide
          trogdor Julian Ridden added a comment -

          Can I add another alternative.

          I agree headers are good for mobile devices. But I disagree on forcing this on everyone as it really does suck and cause confusion on desktops.

          So how abut this alternative. We add the .hidden-desktop to the header and footer divs in Bootstrap. This will then only display the header/footer on mobile devices but not the desktop.

          Julian

          Show
          trogdor Julian Ridden added a comment - Can I add another alternative. I agree headers are good for mobile devices. But I disagree on forcing this on everyone as it really does suck and cause confusion on desktops. So how abut this alternative. We add the .hidden-desktop to the header and footer divs in Bootstrap. This will then only display the header/footer on mobile devices but not the desktop. Julian
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Thanks guys. Both of your solutions seem reasonable. I'll tend personally for Julian's suggestion, I thought about that too at first when looking at the issue. Mary let us know what you think, in the mean while I'll make this change and resend it to integration. Cheers.

          Show
          jerome Jérôme Mouneyrac added a comment - Thanks guys. Both of your solutions seem reasonable. I'll tend personally for Julian's suggestion, I thought about that too at first when looking at the issue. Mary let us know what you think, in the mean while I'll make this change and resend it to integration. Cheers.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          Hum as we can not use the bootstrap responsive size (popup are phone sized on desktop), then I had to call the detect browser function. I'll send it for peer-review as the code changed a bit.

          To make the peer-review easy, the change between columns1.php and popup.php are:
          https://github.com/mouneyrac/moodle/blob/1f802c17c022a649e5fa51ce903240de61d15f09/theme/bootstrapbase/layout/popup.php#L30-40
          https://github.com/mouneyrac/moodle/blob/1f802c17c022a649e5fa51ce903240de61d15f09/theme/bootstrapbase/layout/popup.php#L83

          Show
          jerome Jérôme Mouneyrac added a comment - - edited Hum as we can not use the bootstrap responsive size (popup are phone sized on desktop), then I had to call the detect browser function. I'll send it for peer-review as the code changed a bit. To make the peer-review easy, the change between columns1.php and popup.php are: https://github.com/mouneyrac/moodle/blob/1f802c17c022a649e5fa51ce903240de61d15f09/theme/bootstrapbase/layout/popup.php#L30-40 https://github.com/mouneyrac/moodle/blob/1f802c17c022a649e5fa51ce903240de61d15f09/theme/bootstrapbase/layout/popup.php#L83
          Hide
          lazydaisy Mary Evans added a comment -

          Jerome, I think you are missing the point that Julian made about adding the class="hidden-desktop" to both the header and the footer in the popup.php file, which will work automatically and does not need any extra code.

          Show
          lazydaisy Mary Evans added a comment - Jerome, I think you are missing the point that Julian made about adding the class="hidden-desktop" to both the header and the footer in the popup.php file, which will work automatically and does not need any extra code.
          Hide
          jerome Jérôme Mouneyrac added a comment -

          Hi Mary, I did check and understand the hidden-desktop class. The problem here is that if you use hidden-desktop, then on desktop with a small popup (most likely all popup) you'll see the header/footer.

          Show
          jerome Jérôme Mouneyrac added a comment - Hi Mary, I did check and understand the hidden-desktop class. The problem here is that if you use hidden-desktop, then on desktop with a small popup (most likely all popup) you'll see the header/footer.
          Hide
          lazydaisy Mary Evans added a comment -

          I'll try and test this out and get back to you.

          Show
          lazydaisy Mary Evans added a comment - I'll try and test this out and get back to you.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          To accelerate the process, I'll send the issue back to integration review but don't hesitate to give your opinion Mary (or anyone else), thanks for having a look.

          *For integrators*
          the fix is a copy of columns1.php into popup.php with the following changes between columns1.php and popup.php:

          Show
          jerome Jérôme Mouneyrac added a comment - - edited To accelerate the process, I'll send the issue back to integration review but don't hesitate to give your opinion Mary (or anyone else), thanks for having a look. * For integrators * the fix is a copy of columns1.php into popup.php with the following changes between columns1.php and popup.php: https://github.com/mouneyrac/moodle/blob/1f802c17c022a649e5fa51ce903240de61d15f09/theme/bootstrapbase/layout/popup.php#L30-40 https://github.com/mouneyrac/moodle/blob/1f802c17c022a649e5fa51ce903240de61d15f09/theme/bootstrapbase/layout/popup.php#L83
          Hide
          stronk7 Eloy Lafuente (stronk7) 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
          stronk7 Eloy Lafuente (stronk7) 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
          samhemelryk Sam Hemelryk added a comment -

          Thanks Jerome - this has been integrated now.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Jerome - this has been integrated now.
          Hide
          dobedobedoh Andrew Nicols added a comment - - edited

          Seen on 25 when the popup opens:

          Fatal error: Class 'core_useragent' not found in /home/nicols/Public/banana.local/data/integration_25/moodle/theme/bootstrapbase/layout/popup.php on line 33
          

          Show
          dobedobedoh Andrew Nicols added a comment - - edited Seen on 25 when the popup opens: Fatal error: Class 'core_useragent' not found in /home/nicols/Public/banana.local/data/integration_25/moodle/theme/bootstrapbase/layout/popup.php on line 33
          Hide
          jerome Jérôme Mouneyrac added a comment -

          argh... thanks Andrew, looking at it.

          Show
          jerome Jérôme Mouneyrac added a comment - argh... thanks Andrew, looking at it.
          Hide
          jerome Jérôme Mouneyrac added a comment - - edited

          I fixed the 25 error + I removed the extra padding top of the body for both popup in 25/master.

          Show
          jerome Jérôme Mouneyrac added a comment - - edited I fixed the 25 error + I removed the extra padding top of the body for both popup in 25/master.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Jerome - MOODLE_25_STABLE changes pulled in now.

          Andrew Nicols back to you

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Jerome - MOODLE_25_STABLE changes pulled in now. Andrew Nicols back to you
          Hide
          dobedobedoh Andrew Nicols added a comment -

          Working now. Retested on master, and 25. Success!

          Show
          dobedobedoh Andrew Nicols added a comment - Working now. Retested on master, and 25. Success!
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          ...
          But still, I thank you, for you made me stronger…

          Stronger as the beast that roar in the wild
          Stronger as the storm across the ocean
          Stronger as the diamond that won’t break
          Stronger enough to take all heart aches….

          I thank you my friend, for you made me stronger…

          ---- Juneah Landicho

          Closing as fixed. Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                13/Jan/14