Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.4
    • Component/s: Accessibility
    • Labels:
    • Testing Instructions:
      Hide
      1. Goto course>reports>logs and select all days so you can have multiple pages of result
      2. goto any page other than 1 and make sure the title tag in the page displays the page number
      3. Goto siteadmin>reports>logs and select all days so you can have multiple pages of result
      4. goto any page other than 1 and make sure the title tag in the page displays the page number
      Show
      Goto course>reports>logs and select all days so you can have multiple pages of result goto any page other than 1 and make sure the title tag in the page displays the page number Goto siteadmin>reports>logs and select all days so you can have multiple pages of result goto any page other than 1 and make sure the title tag in the page displays the page number
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-35225-master

      Description

      Multipage items (e.g. Book, Activity Reports, Calendar) should have their page titles change to indicate to the user which page of a resource they are viewing instead of always keeping the same title.

      Greg's comment in MDL-30896 suggest
      For the reports (like the logs) the page title should be something more like
      "Logs: Page 3" or
      "Logs: 1/20/12 12:27PM - 1/20/12 4:12PM"

      instead of every page just saying "Logs"

        Gliffy Diagrams

          Activity

          Hide
          ankit_frenz Ankit Agarwal added a comment -

          will upload stable branches once peer-review is done.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - will upload stable branches once peer-review is done. Thanks
          Hide
          fred Frédéric Massart added a comment - - edited

          Hi Ankit, although your patch looks good, I highly discourage the concatenation of strings, especially when using a colon which is not used in some languages, or require other an extra space before (in French for instance). I know that most of the area in Moodle do not take care of this, but we should definitely think about it. Cheers! (Edit: forgot to mention the unneeded new line on line 121)

          Show
          fred Frédéric Massart added a comment - - edited Hi Ankit, although your patch looks good, I highly discourage the concatenation of strings, especially when using a colon which is not used in some languages, or require other an extra space before (in French for instance). I know that most of the area in Moodle do not take care of this, but we should definitely think about it. Cheers! (Edit: forgot to mention the unneeded new line on line 121)
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Hi Fred,
          Thanks for your feedback.

          Yes normally we donot concatenate strings in Moodle. But for page titles, I am afraid it needs to be done. Afaik, I confirmed this with Dan, as well earlier, that in case of titles it is alright, but should never be done in content.

          Will update the patch to remove extra line, and push up stable branches.

          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Hi Fred, Thanks for your feedback. Yes normally we donot concatenate strings in Moodle. But for page titles, I am afraid it needs to be done. Afaik, I confirmed this with Dan, as well earlier, that in case of titles it is alright, but should never be done in content. Will update the patch to remove extra line, and push up stable branches. Thanks
          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
          ankit_frenz Ankit Agarwal added a comment -

          rebased.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - rebased. Thanks
          Hide
          poltawski Dan Poltawski added a comment -

          Hmm, well this is a simple issue, but i'm afraid I have some reservations about this.

          OK, so we now have a 'unique page title', but we've also introduced a new string. That is not without its cost:

          1. Our translators struggle to keep up with our new strings: http://download.moodle.org/langpack/2.3/
          2. Its almost certain that once this patch is applied, in most languages we'll be adding another untranslated string to the title
          3. Is adding a [probably] untranslated string really improving accessibility?
          Show
          poltawski Dan Poltawski added a comment - Hmm, well this is a simple issue, but i'm afraid I have some reservations about this. OK, so we now have a 'unique page title', but we've also introduced a new string. That is not without its cost: Our translators struggle to keep up with our new strings: http://download.moodle.org/langpack/2.3/ Its almost certain that once this patch is applied, in most languages we'll be adding another untranslated string to the title Is adding a [probably] untranslated string really improving accessibility?
          Hide
          poltawski Dan Poltawski added a comment -

          If there is no existing string, then I think it would be better to apply this to master only.

          Show
          poltawski Dan Poltawski added a comment - If there is no existing string, then I think it would be better to apply this to master only.
          Hide
          ankit_frenz Ankit Agarwal added a comment -

          Hi Dan,
          Yeah I agree with your explanations. The reason, I provided stable branches as well, because we are supposed to treat accessibility improvements as bugs. If you think the new string wont be useful in stables, please feel free to integrate in master only.
          Thanks

          Show
          ankit_frenz Ankit Agarwal added a comment - Hi Dan, Yeah I agree with your explanations. The reason, I provided stable branches as well, because we are supposed to treat accessibility improvements as bugs. If you think the new string wont be useful in stables, please feel free to integrate in master only. Thanks
          Hide
          poltawski Dan Poltawski added a comment -

          I'm not a massive fan of this string concaination, but it matches existing behaviour, so i've integrated this.

          2.4 only, because IMO its better to have it in exsiting behaviour than likely untranslated strings.

          Show
          poltawski Dan Poltawski added a comment - I'm not a massive fan of this string concaination, but it matches existing behaviour, so i've integrated this. 2.4 only, because IMO its better to have it in exsiting behaviour than likely untranslated strings.
          Hide
          timb Tim Barker added a comment -

          Congrats Ankit, your bug is fixed and has passed the final hurdle, well done

          Show
          timb Tim Barker added a comment - Congrats Ankit, your bug is fixed and has passed the final hurdle, well done
          Hide
          poltawski Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          poltawski Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                3/Dec/12