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
    • Rank:
      44947

      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.

        Activity

        Hide
        Amy Groshek added a comment -

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

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

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

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

        Thanks for working on this, guys.

        Show
        Michael de Raadt added a comment - Thanks for working on this, guys.
        Hide
        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
        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
        Justin Filip added a comment -

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

        Thanks!

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

        Thanks Justin,

        That all looks good to me.

        Please submit for integration review.

        Show
        Adrian Greeve added a comment - Thanks Justin, That all looks good to me. Please submit for integration review.
        Hide
        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
        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
        Adrian Greeve added a comment -

        Sorry, I didn't realise.

        Now submitted for integration.

        Show
        Adrian Greeve added a comment - Sorry, I didn't realise. Now submitted for integration.
        Hide
        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
        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
        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
        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
        Justin Filip added a comment -

        Hi Dan,

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

        Cheers!

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

        Thanks Justin, integrated to 22, 23 and master.

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

        It passes. Tested in 22, 23 and master

        Show
        David Monllaó added a comment - It passes. Tested in 22, 23 and master
        Hide
        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
        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: