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

remove border=1 from device detection regex table

    Details

    • Testing Instructions:
      Hide

      Visit:

      /moodle/admin/settings.php?section=themesettings

      using Clean theme.

      The table in Device detection regular expressions should look nicer, without ugly black borders on the bottom and vertical dividers.

      Show
      Visit: /moodle/admin/settings.php?section=themesettings using Clean theme. The table in Device detection regular expressions should look nicer, without ugly black borders on the bottom and vertical dividers.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-44168-master

      Description

      There's a border=1 set on the device detection regex table in admin -> theme settings:

      /moodle/admin/settings.php?section=themesettings

      This isn't usually visible because Moodle includes YUI CSS that add a border to every single td and th, and then Moodle CSS fixes that by including CSS that removes borders from every single td and th.

      The Bootstrapbase/Clean theme doesn't use the YUI CSS, and has removed the workaround that removes all borders from tables (in order to fix MDL-27774) and now you get the border=1 on this table showing through again.

      When looking at this I noticed that the code to write out this table has a another bug. Normally the first row has r0 and r1 alternating, while each cell is c0, then c1, then c2 etc. This code adds c0 to each cell on the first row, c1 to each cell on the second row, c2 on each cell on the third row etc.

      I don't know if it's worth messing with, I don't think it has any actual effect, but I thought I'd note it anyway.

        Gliffy Diagrams

          Activity

          Hide
          cibot CiBoT added a comment -

          Results for MDL-44168

          • Remote repository: git://github.com/ds125v/moodle.git
          Show
          cibot CiBoT added a comment - Results for MDL-44168 Remote repository: git://github.com/ds125v/moodle.git Remote branch wip- MDL-44168 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1381 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1381/artifact/work/smurf.html Remote branch wip- MDL-44168 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1382 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1382/artifact/work/smurf.html
          Hide
          lazydaisy Mary Evans added a comment -

          Looks OK to me David, but wondering if the problem is not in this but in the way the detection box has been coded. The Admin may well add borders where borders are needed, grader for example?

          Show
          lazydaisy Mary Evans added a comment - Looks OK to me David, but wondering if the problem is not in this but in the way the detection box has been coded. The Admin may well add borders where borders are needed, grader for example?
          Hide
          bawjaws David Scotson added a comment -

          I think the old way of adding borders (i.e. border="1" cellspacing="4" etc.) is probably always the wrong way do things now, but there's no point changing them unless they're causing trouble. I checked for other examples and there's a small number of border=1, the vast majority are border=0, which is probably less of an issue.

          While doing that I also found some .less that was supposed to overrule this in Bootstrapbase, which can be removed with this change, so I'll update the commit.

          Show
          bawjaws David Scotson added a comment - I think the old way of adding borders (i.e. border="1" cellspacing="4" etc.) is probably always the wrong way do things now, but there's no point changing them unless they're causing trouble. I checked for other examples and there's a small number of border=1, the vast majority are border=0, which is probably less of an issue. While doing that I also found some .less that was supposed to overrule this in Bootstrapbase, which can be removed with this change, so I'll update the commit.
          Hide
          lazydaisy Mary Evans added a comment -

          David, you really need to update your Moodle then Rebase these branches...currently you are 11 days behind! Also come Thurday you will need to update and rebase again!

          Show
          lazydaisy Mary Evans added a comment - David, you really need to update your Moodle then Rebase these branches...currently you are 11 days behind! Also come Thurday you will need to update and rebase again!
          Hide
          lazydaisy Mary Evans added a comment - - edited

          The CSS is wrong in Bootstrapbase that is why only half of the border is still showing up.

          The CSS should be...

          #admin-devicedetectregex .generaltable th,
          #admin-devicedetectregex .generaltable td {
              border: 0 none;
          }

          Show
          lazydaisy Mary Evans added a comment - - edited The CSS is wrong in Bootstrapbase that is why only half of the border is still showing up. The CSS should be... #admin-devicedetectregex .generaltable th, #admin-devicedetectregex .generaltable td { border: 0 none; }
          Hide
          lazydaisy Mary Evans added a comment -

          David, I really think that this can be fixed in Bootstrap. HQ are very particular about changing HTML in CORE so if I were you I would drop that idea and concentrate on getting Bootstrapbase LESS fxed for this bug.

          Show
          lazydaisy Mary Evans added a comment - David, I really think that this can be fixed in Bootstrap. HQ are very particular about changing HTML in CORE so if I were you I would drop that idea and concentrate on getting Bootstrapbase LESS fxed for this bug.
          Hide
          lazydaisy Mary Evans added a comment - - edited

          Ignore my last comment, I thought this was a general admin table, I totally missed which page you were fixing.

          If you can update you Moodle master and STABLE branches and then rebase these tracker branches I can push them for Integration Review.

          Show
          lazydaisy Mary Evans added a comment - - edited Ignore my last comment, I thought this was a general admin table, I totally missed which page you were fixing. If you can update you Moodle master and STABLE branches and then rebase these tracker branches I can push them for Integration Review.
          Hide
          bawjaws David Scotson added a comment -

          That should be done now, thanks.

          Show
          bawjaws David Scotson added a comment - That should be done now, thanks.
          Hide
          lazydaisy Mary Evans added a comment -

          Pushing for Integration Review

          Show
          lazydaisy Mary Evans added a comment - Pushing for Integration Review
          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
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          damyon Damyon Wiese added a comment -

          Thanks David. I agree it makes sense to remove this.

          Integrated to 25, 26 and master (I cherry-picked to 25). Please backport bug fixes to all supported branches all the time.

          Show
          damyon Damyon Wiese added a comment - Thanks David. I agree it makes sense to remove this. Integrated to 25, 26 and master (I cherry-picked to 25). Please backport bug fixes to all supported branches all the time.
          Hide
          fred Frédéric Massart added a comment -

          Passing, thanks.

          Show
          fred Frédéric Massart added a comment - Passing, thanks.
          Hide
          damyon Damyon Wiese added a comment -

          Parallel programmParallel programming is harding is hard

          Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

          Show
          damyon Damyon Wiese added a comment - Parallel programmParallel programming is harding is hard Thanks for reporting/fixing/testing/improving Moodle. This issue has now been released.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Mar/14