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

Database: Blocks editing button wrapped in <table> which interferes with theming

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide

      Requirements

      • a user with course editing capabilities
      • a course with a Database activity module instance

      Instructions

      1. access a Moodle course as a user with editing capabilities and editing mode turned off
      2. access Database activity module instance
      3. using browser developer tools, inspect the element for the block that says Blocks editing on
      4. click on the Blocks editing on button
      5. using browser developer tools, inspect the element for the block that says Blocks editing off
      6. click on the Blocks editing off button

      Expected results

      • (2) – a button containing the text Blocks editing on should appear on the far right of the navigation bar
      • (3) – the button should be contained only within nested DIV elements (no TABLE HTML should be present around the block)
      • (4) – the page should reload and the block text should change to Blocks editing off and block editing controls should appear on any blocks already on the page. In addition, the Add a block block should appear on the page.
      • (5) – the button should be contained only within nested DIV elements (no TABLE HTML should be present around the block)
      • (6) – the page should reload and the block text should change to Blocks editing on and block editing controls should not appear on any of the blocks already on the page. The Add a block block should not appear on the page.
      Show
      Requirements a user with course editing capabilities a course with a Database activity module instance Instructions access a Moodle course as a user with editing capabilities and editing mode turned off access Database activity module instance using browser developer tools, inspect the element for the block that says Blocks editing on click on the Blocks editing on button using browser developer tools, inspect the element for the block that says Blocks editing off click on the Blocks editing off button Expected results (2) – a button containing the text Blocks editing on should appear on the far right of the navigation bar (3) – the button should be contained only within nested DIV elements (no TABLE HTML should be present around the block) (4) – the page should reload and the block text should change to Blocks editing off and block editing controls should appear on any blocks already on the page. In addition, the Add a block block should appear on the page. (5) – the button should be contained only within nested DIV elements (no TABLE HTML should be present around the block) (6) – the page should reload and the block text should change to Blocks editing on and block editing controls should not appear on any of the blocks already on the page. The Add a block block should not appear on the page.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36170_master

      Description

      In View List, View Single, and Search tabs of database module, Blocks editing on/off button is wrapped in a table. This increases the height of the navbar and requires targeted CSS fixes.

        Gliffy Diagrams

          Activity

          Hide
          agroshek Amy Groshek added a comment -

          Table markup begins on line 303 of data module's view.php.

          Show
          agroshek Amy Groshek added a comment - Table markup begins on line 303 of data module's view.php.
          Hide
          jfilip Justin Filip added a comment -

          The commit from the 2.3 branch was cherry-picked into the 2.2 and master branches here.

          Show
          jfilip Justin Filip added a comment - The commit from the 2.3 branch was cherry-picked into the 2.2 and master branches here.
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for working on this, guys.

          Show
          salvetore Michael de Raadt added a comment - Thanks for working on this, guys.
          Hide
          abgreeve Adrian Greeve added a comment -

          [N] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [-] Security
          [-] Documentation
          [N] Git
          [Y] Sanity check

          Hi Justin,

          Thanks for fixing this up, the replacement code is much nicer than the original, just a few minor admin things:

          1. mod/data/view.php line 303 We don't really need a comment here. The comment seems out of place with out the previous code to look at, and we have the commit message to view the change that has been made.
          2. I'm not a huge fan of the short hand if statement on lines 304 and 305. I know this is just copying the existing code, but it makes the lines more difficult to understand and it creates long lines which we are trying to avoid. This is just a personal preference, so you don't need to make a change here.
          3. If you could provide some testing instructions that would be wonderful. Even if it is just to use firebug to inspect the code and make sure that it isn't in a table.
          4. Your git commit message is missing the component section. http://docs.moodle.org/dev/Peer_reviewing_checklist#Git
          Show
          abgreeve Adrian Greeve added a comment - [N] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [-] Security [-] Documentation [N] Git [Y] Sanity check Hi Justin, Thanks for fixing this up, the replacement code is much nicer than the original, just a few minor admin things: mod/data/view.php line 303 We don't really need a comment here. The comment seems out of place with out the previous code to look at, and we have the commit message to view the change that has been made. I'm not a huge fan of the short hand if statement on lines 304 and 305. I know this is just copying the existing code, but it makes the lines more difficult to understand and it creates long lines which we are trying to avoid. This is just a personal preference, so you don't need to make a change here. If you could provide some testing instructions that would be wonderful. Even if it is just to use firebug to inspect the code and make sure that it isn't in a table. Your git commit message is missing the component section. http://docs.moodle.org/dev/Peer_reviewing_checklist#Git
          Hide
          jfilip Justin Filip added a comment -

          Implemented changes based on Adrian's feedback. Resubmitted for peer review.

          Thanks!

          Show
          jfilip Justin Filip added a comment - Implemented changes based on Adrian's feedback. Resubmitted for peer review. Thanks!
          Hide
          abgreeve Adrian Greeve added a comment -

          Thanks Justin,

          That all looks good to me.

          Please submit for integration review.

          Show
          abgreeve Adrian Greeve added a comment - Thanks Justin, That all looks good to me. Please submit for integration review.
          Hide
          jfilip Justin Filip added a comment -

          Adrian, I don't have permission to submit this for integration review. Can you perform that part of the work-flow for me?

          Show
          jfilip Justin Filip added a comment - Adrian, I don't have permission to submit this for integration review. Can you perform that part of the work-flow for me?
          Hide
          abgreeve Adrian Greeve added a comment -

          Sorry, I didn't realise.

          Now submitted for integration.

          Show
          abgreeve Adrian Greeve added a comment - Sorry, I didn't realise. Now submitted for integration.
          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
          poltawski Dan Poltawski added a comment -

          Hi Justin,

          This looks find, but the git history is a bit messy with reverts.

          Are you able to rebase the commits on your branches into a single commit?

          cheers,
          Dan

          Show
          poltawski Dan Poltawski added a comment - Hi Justin, This looks find, but the git history is a bit messy with reverts. Are you able to rebase the commits on your branches into a single commit? cheers, Dan
          Hide
          jfilip Justin Filip added a comment -

          Hi Dan,

          I've updated the branches and rebased the latest upstream into them just in case.

          Cheers!

          Show
          jfilip Justin Filip added a comment - Hi Dan, I've updated the branches and rebased the latest upstream into them just in case. Cheers!
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Justin, integrated to 22, 23 and master.

          Show
          poltawski Dan Poltawski added a comment - Thanks Justin, integrated to 22, 23 and master.
          Hide
          dmonllao David Monllaó added a comment -

          It passes. Tested in 22, 23 and master

          Show
          dmonllao David Monllaó added a comment - It passes. Tested in 22, 23 and master
          Hide
          poltawski Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          poltawski Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

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

              Dates

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