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

          Attachments

            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