Details

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

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Mary Evans added a comment -

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

          Show
          Mary Evans added a comment - Andrew can you peer review this for Bas? If not let me know. Thanks.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
          Hide
          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
          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
          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
          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: