Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Themes
    • Labels:

      Description

      The Bootstrap layout file contains a wrapper div for the main content with id = "region-bs-main". This is inconsistent with other themes and can break plugin that use JavaScript to identify the main region.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            lazydaisy Mary Evans added a comment - - edited

            Just added the Pull from Repository and Pull Master Branch sections (see above), also edited Pull master Diff URL you ommited compare/master...

            Can you add relevant Testing Instructions in the editable section above (edit link it top left)?

            Thanks

            Show
            lazydaisy Mary Evans added a comment - - edited Just added the Pull from Repository and Pull Master Branch sections (see above), also edited Pull master Diff URL you ommited compare/master... Can you add relevant Testing Instructions in the editable section above (edit link it top left)? Thanks
            Hide
            basbrands Bas Brands added a comment -

            I am not sure how to test write test instructions for this. the id's "region-main" or "region-bs-main" are not used in the bootstrap theme css.

            If filed this issue after a report on the theme_bootstrap plugin page:

            "Roland Sherwood has commented on a plugin you're contributing to :

            Thanks for the reply Bas. I actually just heard back from the Text to Speech
            block developer also, who thinks the issue may be caused by Bootstrap's
            renaming of the region-main element:

            "The TTS block search for the element with ID="region-main" in the HTML
            structure. Unfortunately, bootstrap rename this div ID with "region-bs-main" so
            the TTS doesn't find the element and stop here."

            I did find a bit of JavaScript in Moodle core that uses the region-main tag. It is in moodle-core-blocks.js:

            CSS.REGIONMAIN = 'region-main'

            // Add relevant classes and ID to 'content' block region on My Home page.
            var myhomecontent = Y.Node.all('body#'CSS.MYINDEX' #'CSS.REGIONMAIN' > .'+CSS.REGIONCONTENT);
            if (myhomecontent.size() > 0)

            { var contentregion = myhomecontent.item(0); contentregion.addClass(CSS.BLOCKREGION); contentregion.set('id', CSS.REGIONCONTENT); contentregion.one('div').addClass(CSS.REGIONCONTENT); }

            Which adds block-region and region-content classes on some elements. I don't know why.

            Show
            basbrands Bas Brands added a comment - I am not sure how to test write test instructions for this. the id's "region-main" or "region-bs-main" are not used in the bootstrap theme css. If filed this issue after a report on the theme_bootstrap plugin page: "Roland Sherwood has commented on a plugin you're contributing to : Thanks for the reply Bas. I actually just heard back from the Text to Speech block developer also, who thinks the issue may be caused by Bootstrap's renaming of the region-main element: "The TTS block search for the element with ID="region-main" in the HTML structure. Unfortunately, bootstrap rename this div ID with "region-bs-main" so the TTS doesn't find the element and stop here." I did find a bit of JavaScript in Moodle core that uses the region-main tag. It is in moodle-core-blocks.js: CSS.REGIONMAIN = 'region-main' // Add relevant classes and ID to 'content' block region on My Home page. var myhomecontent = Y.Node.all('body#' CSS.MYINDEX ' #' CSS.REGIONMAIN ' > .'+CSS.REGIONCONTENT); if (myhomecontent.size() > 0) { var contentregion = myhomecontent.item(0); contentregion.addClass(CSS.BLOCKREGION); contentregion.set('id', CSS.REGIONCONTENT); contentregion.one('div').addClass(CSS.REGIONCONTENT); } Which adds block-region and region-content classes on some elements. I don't know why.
            Hide
            lazydaisy Mary Evans added a comment -

            Hi Bas,

            Why did you remove the Triaged label? And the links to your github patch? There was nothing wrong with it.

            As for the JavaScript adding block-region and region-content I think that's do with moving blocks, drag-n-drop etc.

            I'll add Andrew and Ruslan who know more about these things and can explain better.

            Show
            lazydaisy Mary Evans added a comment - Hi Bas, Why did you remove the Triaged label? And the links to your github patch? There was nothing wrong with it. As for the JavaScript adding block-region and region-content I think that's do with moving blocks, drag-n-drop etc. I'll add Andrew and Ruslan who know more about these things and can explain better.
            Hide
            lazydaisy Mary Evans added a comment -

            Andrew can you peer review this for Bas? If not let me know. Thanks.

            Show
            lazydaisy Mary Evans added a comment - Andrew can you peer review this for Bas? If not let me know. Thanks.
            Hide
            lazydaisy Mary Evans added a comment -

            Bas the test instructions are only a guide to seeing if this patch causes any problems. As it's not an easy thing to test I have asked Andrew to Peer Review it so that it gets approval before being integrated.

            Show
            lazydaisy Mary Evans added a comment - Bas the test instructions are only a guide to seeing if this patch causes any problems. As it's not an easy thing to test I have asked Andrew to Peer Review it so that it gets approval before being integrated.
            Hide
            basbrands Bas Brands added a comment -

            Hi Mary.

            I wasn't aware of removing anything. I have tried using the JIRA Client for the tracker issues, perhaps that was the problem. Do you know if using the JIRA Client is advisable?

            Thanks

            Show
            basbrands Bas Brands added a comment - Hi Mary. I wasn't aware of removing anything. I have tried using the JIRA Client for the tracker issues, perhaps that was the problem. Do you know if using the JIRA Client is advisable? Thanks
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'm happy to peer review this, but I'm not sure what this is necessarily solving. I assume that this is intended to add block drag/drop back, but block drag and drop isn't working with this enabled so I guess there's more to do?

            The patch itself looks fine and makes sense. If it isn't solving a specific thing (e.g. block drag/drop) then I think it's fine; otherwise it needs some testing instructions.

            Show
            dobedobedoh Andrew Nicols added a comment - I'm happy to peer review this, but I'm not sure what this is necessarily solving. I assume that this is intended to add block drag/drop back, but block drag and drop isn't working with this enabled so I guess there's more to do? The patch itself looks fine and makes sense. If it isn't solving a specific thing (e.g. block drag/drop) then I think it's fine; otherwise it needs some testing instructions.
            Hide
            lazydaisy Mary Evans added a comment -

            I think renaming the region-bs-main to region-main makes sense too. So lets do it and if problems arise we can deal with them.

            In the mean time we can try and get drag and drop working.

            Show
            lazydaisy Mary Evans added a comment - I think renaming the region-bs-main to region-main makes sense too. So lets do it and if problems arise we can deal with them. In the mean time we can try and get drag and drop working.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Tested and passed.

            To test I checked the display of the bootstrap page on several themes and verified that the blocks JS does in fact pick up region content areas when executing.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Tested and passed. To test I checked the display of the bootstrap page on several themes and verified that the blocks JS does in fact pick up region content areas when executing. Many thanks Sam
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I feel myself really alone tonight! So was time to push your fixes upstream!

            "Lest we forget. We will remember them."

            Thanks and ciao!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

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