Moodle
  1. Moodle
  2. MDL-31824

Calendar "month" view - HTML classes are inconsistent

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.7, 2.4
    • Fix Version/s: 2.4
    • Component/s: Calendar
    • Labels:
    • Environment:
      MySQL, Windows Server 2003, PHP 5.3.8 (VC9), Apache 2.2.21.0 (VC9)
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Visit moodle/calendar/view.php?view=month.

      • Check the classes applied to days in the the current month and days in any other month.
      • On the current month all days have the class "nottoday" (except the actual current day)
      • The current day should have the class "today"
      • Apply a few of the main themes, and check the calendar to ensure it isn't broken.
      Show
      Visit moodle/calendar/view.php?view=month. Check the classes applied to days in the the current month and days in any other month. On the current month all days have the class "nottoday" (except the actual current day) The current day should have the class "today" Apply a few of the main themes, and check the calendar to ensure it isn't broken.
    • Workaround:
      Hide

      Patches

      Show
      Patches
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-31824-master
    • Rank:
      38462

      Description

      When looking at calendar/view.php?view=month the HTML classes are inconsistent with other calendar views and are more generally speaking just broken:

      • Day cells are rendered as "today" or "nottoday" to signify blank cells. By comparison other calendar views use "day" or "dayblank" and then use "today" to highlight the actual date today.
      • When viewing a month other than the current month every day renders with a "nottoday" class.

      Main problem appear to be caused by an if statement in calendar/renderer.php at around line 520:

      // Special visual fx for today
      if($display->thismonth && $calendar->day == $calendar->day) {
      

      of course $calendar->day will ALWAYS equal itself so it returns every day of the current month as "today".

      1. calendar-renderer.patch
        0.6 kB
        Isuru Madushanka Weerarathna
      2. MDL-31824.patch
        0.9 kB
        Mark Ward
      3. MDL-31824 PROPER.patch
        1 kB
        Mark Ward

        Issue Links

          Activity

          Hide
          Isuru Madushanka Weerarathna added a comment -

          Hi,

          I would like to work on this minor bug. But I have a question. Is fixing this bug means, it should append to cell class 'today' only for today date and all the other dates' (including current month) cell class must be 'nottoday' (in month view)?

          -Thanks

          Show
          Isuru Madushanka Weerarathna added a comment - Hi, I would like to work on this minor bug. But I have a question. Is fixing this bug means, it should append to cell class 'today' only for today date and all the other dates' (including current month) cell class must be 'nottoday' (in month view)? -Thanks
          Hide
          Isuru Madushanka Weerarathna added a comment -

          Hi,

          I have attached a patch here which may resolve this inconsistency.

          -Thanks

          Show
          Isuru Madushanka Weerarathna added a comment - Hi, I have attached a patch here which may resolve this inconsistency. -Thanks
          Hide
          Mark Ward added a comment -

          Hi Isuru,

          Thanks for the patch, that will handle the problem with every day of this month being "today" and every day of other months being "nottoday".

          The remaining issue is that blank cells which are before and after the days of a month do not have an identifiable class. Other calendar views give these cells the class "dayblank" which we can apply to this view at around line 460. I've added a second patch which includes your changes but also adds this new class to the blank days.

          Show
          Mark Ward added a comment - Hi Isuru, Thanks for the patch, that will handle the problem with every day of this month being "today" and every day of other months being "nottoday". The remaining issue is that blank cells which are before and after the days of a month do not have an identifiable class. Other calendar views give these cells the class "dayblank" which we can apply to this view at around line 460. I've added a second patch which includes your changes but also adds this new class to the blank days.
          Hide
          Mark Ward added a comment -

          Whoops, missed the blank days that come after the month. Use MDL-31824 PROPER.patch.

          Show
          Mark Ward added a comment - Whoops, missed the blank days that come after the month. Use MDL-31824 PROPER.patch.
          Hide
          Jason Fowler added a comment -

          Patch applied, will back port it after peer review

          Show
          Jason Fowler added a comment - Patch applied, will back port it after peer review
          Hide
          Jason Fowler added a comment -

          would probably help if I provided pull branches

          Show
          Jason Fowler added a comment - would probably help if I provided pull branches
          Hide
          Mark Ward added a comment -

          Heh, thanks Jason!

          Show
          Mark Ward added a comment - Heh, thanks Jason!
          Hide
          Frédéric Massart added a comment -

          Hi Jason. Your patch looks good, and I think it would have been save to backport it if the line 522 did not need a change. Although, as you're editing it could you add a space between the if and the bracket?

          Feel free to push for integration whenever you like. Cheers!

          Show
          Frédéric Massart added a comment - Hi Jason. Your patch looks good, and I think it would have been save to backport it if the line 522 did not need a change. Although, as you're editing it could you add a space between the if and the bracket? Feel free to push for integration whenever you like. Cheers!
          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
          Jason Fowler added a comment -

          just finished rebasing this

          Show
          Jason Fowler added a comment - just finished rebasing this
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Sorry, I'm strictly reading the proposed patch (not looking to context nor to other scripts) and it looks like we are adding more and more combinations. Right now I can see:

          1- nottoday blank
          2- day nottoday
          3- day today
          4- nottoday dayblank

          And that raises some questions:

          1) what's the difference between 1 and 4. "blank" and "dayblank", for what?
          2) what's the value of having "day" in 2 and 3?
          3) why 2 is missing 'blank' (o 'dayblank') ?

          So really something seems inconsistent there, please clarify it and try to mimic other, more organised, script, if they exist.

          Also... what happens with all core themes using the old classes, for custom styling? All them require review, for sure.

          And finally, if the change is not BC... then I think we won't be backporting this at all or 3rd part themes (out from our control) would stop working suddenly in the middle of stables.

          One possible BC solution could be to add the new, more correct, classes in stables and only replace the old classes in master). Of course such change will also require documentation in 2.4 release notes.

          So, plz:

          1) clarify the naming schema to use it consistently.
          2) look for the impact of that new naming schema in core themes.
          3) provide a BC solution if backporting is desired + documentation about the new naming schema.

          Reopening... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Sorry, I'm strictly reading the proposed patch (not looking to context nor to other scripts) and it looks like we are adding more and more combinations. Right now I can see: 1- nottoday blank 2- day nottoday 3- day today 4- nottoday dayblank And that raises some questions: 1) what's the difference between 1 and 4. "blank" and "dayblank", for what? 2) what's the value of having "day" in 2 and 3? 3) why 2 is missing 'blank' (o 'dayblank') ? So really something seems inconsistent there, please clarify it and try to mimic other, more organised, script, if they exist. Also... what happens with all core themes using the old classes, for custom styling? All them require review, for sure. And finally, if the change is not BC... then I think we won't be backporting this at all or 3rd part themes (out from our control) would stop working suddenly in the middle of stables. One possible BC solution could be to add the new, more correct, classes in stables and only replace the old classes in master). Of course such change will also require documentation in 2.4 release notes. So, plz: 1) clarify the naming schema to use it consistently. 2) look for the impact of that new naming schema in core themes. 3) provide a BC solution if backporting is desired + documentation about the new naming schema. Reopening... ciao
          Hide
          Jason Fowler added a comment -

          None of the elements had classes removed, just reapplied to the correct ones.

          blank is for blank zones on the calendar.

          the purpose of having day is to make it opposed to blank.

          and dayblank should just be blank, I guess

          Show
          Jason Fowler added a comment - None of the elements had classes removed, just reapplied to the correct ones. blank is for blank zones on the calendar. the purpose of having day is to make it opposed to blank. and dayblank should just be blank, I guess
          Hide
          Jason Fowler added a comment -

          I've pulled the BC versions, and fixed the dayblank to be just blank now.

          Show
          Jason Fowler added a comment - I've pulled the BC versions, and fixed the dayblank to be just blank now.
          Hide
          Mark Ward added a comment -

          Hi Both,

          So aside from having fixed the if statement which said "if $calendar->day == $calendar->day" and somehow always came out true (), the patch also addresses inconsistencies between the full calendar and the mini-calendar. The additional classes which were added are all ones which were applied to the mini-calendar and make it possible to style the thing properly.

          The class name "dayblank" is already used within the mini-calendar to signify a blank day. While I agree that plain old "blank" might make more sense, my thought was that we need to keep it consistent with the classes that are being applied on other calendars on Moodle 2 in order to make it easier to apply shared styling.

          Mark

          Show
          Mark Ward added a comment - Hi Both, So aside from having fixed the if statement which said "if $calendar->day == $calendar->day" and somehow always came out true ( ), the patch also addresses inconsistencies between the full calendar and the mini-calendar. The additional classes which were added are all ones which were applied to the mini-calendar and make it possible to style the thing properly. The class name "dayblank" is already used within the mini-calendar to signify a blank day. While I agree that plain old "blank" might make more sense, my thought was that we need to keep it consistent with the classes that are being applied on other calendars on Moodle 2 in order to make it easier to apply shared styling. Mark
          Hide
          Jason Fowler added a comment -

          I think establishing consistency within the calendar is more important than having the two calendars match. I will raise an issue to have the other calendar altered to match this one, as the "dayblank" is incorrect.

          Show
          Jason Fowler added a comment - I think establishing consistency within the calendar is more important than having the two calendars match. I will raise an issue to have the other calendar altered to match this one, as the "dayblank" is incorrect.
          Hide
          Mark Ward added a comment -

          That would require a few changes on every Moodle theme which styles the mini-calendar (which seems to be most of them). Breaking theme compatibility between 2.0>3 to 2.4 seems like a big price to pay just to have an element class name which isn't visible to the end user make more sense.

          Show
          Mark Ward added a comment - That would require a few changes on every Moodle theme which styles the mini-calendar (which seems to be most of them). Breaking theme compatibility between 2.0>3 to 2.4 seems like a big price to pay just to have an element class name which isn't visible to the end user make more sense.
          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
          Jason Fowler added a comment -

          rebased

          Show
          Jason Fowler added a comment - rebased
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Jason Fowler added a comment -

          rebased again

          Show
          Jason Fowler added a comment - rebased again
          Hide
          Aparup Banerjee added a comment -

          Hi both all and everyone,
          first off, i don't see Eloy's questions clarified here so i'll attempt it:

          1- nottoday blank : is being used to signal a non-day cell. a blank cell in the calendar which is Padding. i'd have thought this would be better off called 'padding' or something more to the point like 'notaday' instead of 'blank'. padding cells are obviously 'nottoday' as they are 'notaday' so there seems to be some scope for further cascading rules here.

          2- day nottoday : is a day of the month which is 'nottoday'.
          3- day today : is a day, is today.
          4- nottoday dayblank : which is now "nottoday blank" @ line 556 in current patch - which Jason seems to have fixed up now. The scope for cascading from (1) still applies here.

          so there could be:
          .notaday

          {these are the blank styles}

          .day

          {these carry implicit meaning of not being today and the styles to go with it.}

          .day .today

          {these override style of a day to display as today specially}

          ps: actually even .nottoday could be the default style which is overridden by .day ...


          regarding the matter of :
          a) Mark said: "the patch also addresses inconsistencies between the full calendar and the mini-calendar. "
          b) Jason said: "establishing consistency within the calendar is more important than having the two calendars match"

          i think :

          • if we're doing (a), we can afford to push the renaming of the unclear class name to some minor task for another release if really necessary.
          • if we're doing (b), (i.e. current patch) , then we may as well do it perfectly.. right now (integrated as a chain of dependant fixes even). as it is a lot to expect for all themes to be fixed out there for 2.4, we should make it so that no more changes need to be done for all those themes again in future.

          so, i'll reopen this now for further decision/discussion and changes to be made. i think either way will fix this, but looking at the clock, we're near to freeze soon so unless you've time to fix it (a) would be the way to go for now. I still like b though! (perfectionism)

          ciao!

          Show
          Aparup Banerjee added a comment - Hi both all and everyone, first off, i don't see Eloy's questions clarified here so i'll attempt it: 1- nottoday blank : is being used to signal a non-day cell. a blank cell in the calendar which is Padding. i'd have thought this would be better off called 'padding' or something more to the point like 'notaday' instead of 'blank'. padding cells are obviously 'nottoday' as they are 'notaday' so there seems to be some scope for further cascading rules here. 2- day nottoday : is a day of the month which is 'nottoday'. 3- day today : is a day, is today. 4- nottoday dayblank : which is now "nottoday blank" @ line 556 in current patch - which Jason seems to have fixed up now. The scope for cascading from (1) still applies here. so there could be: .notaday {these are the blank styles} .day {these carry implicit meaning of not being today and the styles to go with it.} .day .today {these override style of a day to display as today specially} ps: actually even .nottoday could be the default style which is overridden by .day ... regarding the matter of : a) Mark said: "the patch also addresses inconsistencies between the full calendar and the mini-calendar. " b) Jason said: "establishing consistency within the calendar is more important than having the two calendars match" i think : if we're doing (a), we can afford to push the renaming of the unclear class name to some minor task for another release if really necessary. if we're doing (b), (i.e. current patch) , then we may as well do it perfectly.. right now (integrated as a chain of dependant fixes even). as it is a lot to expect for all themes to be fixed out there for 2.4, we should make it so that no more changes need to be done for all those themes again in future. so, i'll reopen this now for further decision/discussion and changes to be made. i think either way will fix this, but looking at the clock, we're near to freeze soon so unless you've time to fix it (a) would be the way to go for now. I still like b though! (perfectionism) ciao!
          Hide
          Jason Fowler added a comment -

          I think it'll have to wait then, I'd rather not create code that is messy in the name of compatibility with more messy code if I can avoid it ...

          Show
          Jason Fowler added a comment - I think it'll have to wait then, I'd rather not create code that is messy in the name of compatibility with more messy code if I can avoid it ...
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Aparup Banerjee added a comment -

          For stables (a) would still be the way, perhaps this can be handled with supplied patch (a) and look into more for a (b) solution for master only in a desperate issue?

          Show
          Aparup Banerjee added a comment - For stables (a) would still be the way, perhaps this can be handled with supplied patch (a) and look into more for a (b) solution for master only in a desperate issue?
          Hide
          Jason Fowler added a comment -

          Changing this to an improvement. It's not really a bug, more a difference between two similar areas of moodle that is being consolidated.

          Show
          Jason Fowler added a comment - Changing this to an improvement. It's not really a bug, more a difference between two similar areas of moodle that is being consolidated.
          Hide
          Jason Fowler added a comment -

          I've gone with option a) as Aparup suggested, even though this is no longer going in to the previously released branches

          Show
          Jason Fowler added a comment - I've gone with option a) as Aparup suggested, even though this is no longer going in to the previously released branches
          Hide
          Aparup Banerjee added a comment -

          i think i got auto spelling corrected in my last comment. s/desperate/separate/

          Show
          Aparup Banerjee added a comment - i think i got auto spelling corrected in my last comment. s/desperate/separate/
          Hide
          Frédéric Massart added a comment -

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

          I guess this has had enough iteration to decide what was good to do. My only thought is that this should probably not been backported as the if condition is fixed, and therefore new class names are used, some themes could be a bit broken.

          I'd just add some testing instructions to check whether the calendar and mini-calendar are still nice looking on other themes.

          Thanks guys!

          Show
          Frédéric Massart added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check I guess this has had enough iteration to decide what was good to do. My only thought is that this should probably not been backported as the if condition is fixed, and therefore new class names are used, some themes could be a bit broken. I'd just add some testing instructions to check whether the calendar and mini-calendar are still nice looking on other themes. Thanks guys!
          Hide
          Jason Fowler added a comment -

          the mini-calendar hasn't had any changes made, so it should stay the same. Just added the theme instructions to the test instructions. Pushing for integration now.

          Show
          Jason Fowler added a comment - the mini-calendar hasn't had any changes made, so it should stay the same. Just added the theme instructions to the test instructions. Pushing for integration now.
          Hide
          Mark Ward added a comment -

          Hi Jason,

          "Changing this to an improvement. It's not really a bug, more a difference between two similar areas of moodle that is being consolidated."

          if($calendar->day == $calendar->day) is the bug, and that's why I put "and broken" in the title. Is this being fixed through a separate issue now?

          Show
          Mark Ward added a comment - Hi Jason, "Changing this to an improvement. It's not really a bug, more a difference between two similar areas of moodle that is being consolidated." if($calendar->day == $calendar->day) is the bug, and that's why I put "and broken" in the title. Is this being fixed through a separate issue now?
          Hide
          Jason Fowler added a comment - - edited

          It's fixed for master, but not the stable branches. Will have to discuss the possibility of backporting it to the stable branches with the integrators. I don't see it being a problem.

          Just that if we backport it, we shouldn't be adjusting the CSS for the other elements, as themes may be relying on the class selectors already in place

          Show
          Jason Fowler added a comment - - edited It's fixed for master, but not the stable branches. Will have to discuss the possibility of backporting it to the stable branches with the integrators. I don't see it being a problem. Just that if we backport it, we shouldn't be adjusting the CSS for the other elements, as themes may be relying on the class selectors already in place
          Hide
          Dan Poltawski added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Jason Fowler added a comment -

          Rebased

          Show
          Jason Fowler added a comment - Rebased
          Hide
          Aparup Banerjee 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
          Aparup Banerjee 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 -

          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
          Aparup Banerjee added a comment -

          Thanks for fixing this up Jason.
          This has been integrated into master only.

          notes:
          Jason has created MDL-36367 for backporting this as there are css/theme workarounds for this in stable. (i haven't checked). Please discuss backporting issues there.

          @Mary: added you just for a heads up.

          Show
          Aparup Banerjee added a comment - Thanks for fixing this up Jason. This has been integrated into master only. notes: Jason has created MDL-36367 for backporting this as there are css/theme workarounds for this in stable. (i haven't checked). Please discuss backporting issues there. @Mary: added you just for a heads up.
          Hide
          Ankit Agarwal added a comment -

          I tested with a few random seems, nothing seems broken and works as described.
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - I tested with a few random seems, nothing seems broken and works as described. Passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: