Details

    • Testing Instructions:
      Hide

      I'm not sure if this needs testing as it is just renaming a css class which is predefined for the purpose it is meant for. So the tables in this change will be consistent with other tables in Moodle.

      Show
      I'm not sure if this needs testing as it is just renaming a css class which is predefined for the purpose it is meant for. So the tables in this change will be consistent with other tables in Moodle.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-37613_master

      Description

      Affected files...

      • mod/feedback/analysis.php
      • mod/feedback/analysis_course.php
      • mod/glossary/editcategories.html
      • mod/glossary/editcategories.php
      • mod/glossary/formats.php
      • mod/wiki/pagelib.php (5 instances)

        Gliffy Diagrams

        1. categorieslist_generalbox.png
          18 kB
        2. categorieslist_generaltable.png
          18 kB
        3. feedback_generalbox.png
          28 kB
        4. feedback_generaltable.png
          28 kB
        5. glossaryformat_generalbox.png
          53 kB
        6. glossaryformat_generaltable.png
          54 kB
        7. glossary-generalbox.png
          11 kB
        8. glossary-generaltable.png
          17 kB
        9. wikicontrib_generalbox.png
          26 kB
        10. wikicontrib_generaltable.png
          21 kB

          Activity

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

          RE-BASED

          Show
          Mary Evans added a comment - RE-BASED
          Hide
          Sam Hemelryk added a comment -

          Nice simple tidy up there thanks Mary.

          Show
          Sam Hemelryk added a comment - Nice simple tidy up there thanks Mary.
          Hide
          Marina Glancy added a comment -

          Hi, I opened the page mod/glossary/editcategories and I am not really sure we want to use generaltable there. See my screenshots. I have not tried other pages yet

          Show
          Marina Glancy added a comment - Hi, I opened the page mod/glossary/editcategories and I am not really sure we want to use generaltable there. See my screenshots. I have not tried other pages yet
          Hide
          Marina Glancy added a comment -

          I fail this test because in all cases the class "generalbox" was used for table in order to hide the fact that it is table because we don't want the lines between rows and columns. Changing the css class made the screens look worse.

          See attachments (if file name ends with "generalbox" this is before change, if it ends with "generaltable" this is after change)

          Show
          Marina Glancy added a comment - I fail this test because in all cases the class "generalbox" was used for table in order to hide the fact that it is table because we don't want the lines between rows and columns. Changing the css class made the screens look worse. See attachments (if file name ends with "generalbox" this is before change, if it ends with "generaltable" this is after change)
          Hide
          Mary Evans added a comment -

          Adding Amy Groshek so she can comment on this. I must admit that in this case the generalbox styling looks better. However I'll wait for Amy to decide.

          Show
          Mary Evans added a comment - Adding Amy Groshek so she can comment on this. I must admit that in this case the generalbox styling looks better. However I'll wait for Amy to decide.
          Hide
          Amy Groshek added a comment -

          Mary Evans I see what you're saying. I found this in some other places as well, when I looked at all the screenviews where this would have to be fixed. In many of these cases, I think the change to generaltable is fine overall. Problems occur where tables have been used to lay out items (buttons, for example, that can just be floated) that do not need to be in tables. IMHO, where we bring more consistency and simplicity to our markup, and that results in the page view looking bad, we need to ask whether the page layout is actually being handled correctly.

          Obviously it means even more work... and if there is someone in particular maintaining that mod that person should be involved. But do we want to establish some CSS guidelines and then revisit this with those persons in on the conversation? What's your opinion?

          Show
          Amy Groshek added a comment - Mary Evans I see what you're saying. I found this in some other places as well, when I looked at all the screenviews where this would have to be fixed. In many of these cases, I think the change to generaltable is fine overall. Problems occur where tables have been used to lay out items (buttons, for example, that can just be floated) that do not need to be in tables. IMHO, where we bring more consistency and simplicity to our markup, and that results in the page view looking bad, we need to ask whether the page layout is actually being handled correctly. Obviously it means even more work... and if there is someone in particular maintaining that mod that person should be involved. But do we want to establish some CSS guidelines and then revisit this with those persons in on the conversation? What's your opinion?
          Hide
          Mary Evans added a comment -

          I am inclined to go with your suggestion, Amy, and wait until we have some better guidelines in place and then revising this, and any other areas that needs attention, with the developers who maintain them.

          Luckily enough, this is the only tracker issue that has been queried, which accounts for 10 tables, so this means just over 2/3rds have been been passed.

          Show
          Mary Evans added a comment - I am inclined to go with your suggestion, Amy, and wait until we have some better guidelines in place and then revising this, and any other areas that needs attention, with the developers who maintain them. Luckily enough, this is the only tracker issue that has been queried, which accounts for 10 tables, so this means just over 2/3rds have been been passed.
          Hide
          Mary Evans added a comment - - edited

          @ Marina Glancy

          Thanks for testing this Marina. I agree the pages look better without the line.
          I am just wondering if we could achieve the same look using a grid system for the layout. I'll have to look into this.

          Cheers
          Mary

          Show
          Mary Evans added a comment - - edited @ Marina Glancy Thanks for testing this Marina. I agree the pages look better without the line. I am just wondering if we could achieve the same look using a grid system for the layout. I'll have to look into this. Cheers Mary
          Hide
          Dan Poltawski added a comment -

          Hi,

          We're at the end of the integration cycle for this week and a fix hasn't arrived so i'm going to have to revert this.

          Hopefully wecan get this sorted for next week. Thanks!

          Show
          Dan Poltawski added a comment - Hi, We're at the end of the integration cycle for this week and a fix hasn't arrived so i'm going to have to revert this. Hopefully wecan get this sorted for next week. Thanks!
          Hide
          Mary Evans added a comment -

          Thanks Dan. Closing this as deferred.

          Show
          Mary Evans added a comment - Thanks Dan. Closing this as deferred.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: