-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add better formatting for multilingual titles #1164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me (just reading code, not testing). I assume this is tested with Moodle and some multilingual material that has applied the '|' notation. It seems that parse_localized() has not been used anywhere earlier apart from one manage script, but apparently it has been tested working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good! Usually the comments have used capitalized first letters, but I'm not sure how much we care about that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not fix the (Moodle) gradebook grade item titles at all yet. The code only affects the Moodle activity names shown in the course section pages. The same view classes and methods that you have modified include the grade item creation too. In the Grades section of Moodle, you can still clearly see the titles in the raw multilingual format (which was part of the issue).
I think the code becomes simpler when you replace parse_localized()
with pick_localized()
.
Rebase onto the upstream master branch and fix the git conflicts.
Include all cases of the LTI content selection (Deep Linking. Currently, you have not touched the class LtiSelectCourseView
which corresponds to selecting one whole course.
The teacher may select any one of the following:
- whole course
- whole module
- one chapter (with or without embedded assignments)
- one assignment
8436db0
to
8ce4a26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this looks good now! I will test this a little and merge the PR tomorrow.
8ce4a26
to
949d84e
Compare
I made a small change to the git commit message, hence the force push before merging. |
Fixes #1116
Description
What?
This pull request only shows the English title for modules and exercises, or the default language if the English translation does not exist. This preserves existing behavior for exercises and modules with no translations.
Why?
The titles were hard to read as it had messy formatting, and showed multiple languages.
How?
By using the
parse_localization
function on the title field, check if there are multiple translations of the title. If so, try to choose the English translation. If there is no English, default to whatever the first language is. If there is no translations, then just return the title as is.Fixes #1116
Testing
Remember to add or update unit tests for new features and changes.
What type of test did you run?
I used the O1 course to test multilingual titles, and the default aplus-manual courses for non-multilingual. I ensured that the English titles were chosen correctly for O1, and that behavior did not change at all in aplus-manual.
Did you test the changes in
Think of what is affected by these changes and could become broken
Translation
Programming style
Have you updated the README or other relevant documentation?
Is it Done?
Clean up your git commit history before submitting the pull request!