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
New teacher dashboard bug fixes #27063
Conversation
apps/i18n/common/en_us.json
Outdated
@@ -1316,7 +1317,7 @@ | |||
"syncClever": "Sync students from Clever", | |||
"syncGoogleClassroom": "Sync students from Google Classroom", | |||
"syncingYourStudents": "Syncing Your Students", | |||
"syncingYourStudentsDescription": "If your Code.org section is ever out of date with your list of students in {loginType}, click on the \"Sync students from {loginType}\" button on the \"Manage students\" tab: ", | |||
"syncingYourStudentsDescription": "If your Code.org section is ever out of date with your list of students in {loginType}, click on the \"Sync students from {loginType}\" button on the <a href={url}>\"Manage students\"</a> tab: ", |
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 pattern of allowing HTML inside translated text is scary and I think we should look into alternatives. I know that others in this situation have not liked the options, so there may not be a great answer. I think there might be a way to do it using dangerouslySetInnerHTML
. @Hamms do you know what the best practice is here?
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.
There's no great way to do this just yet, but the best practice for right now is to do this with markdown rather than html and use the UnsafeRenderedMarkdown component.
The long-term plan is to use the redaction/restoration process for markdown to defend against link injection via translations, and for the UnsafeRenderedMarkdown component to be updated to leverage remark to defend against script injection.
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.
thanks! i am using the UnsafeRenderedMarkdown
component, so i think we're good there. i tried to insert the hyperlink using markdown syntax, but it didn't work, so i used HTML instead. i was probably doing it wrong -- what's the best way to insert a markdown hyperlink into a string?
this is what i tried:
"If your Code.org section is ever out of date with your list of students in {loginType}, click on the \"Sync students from {loginType}\" button on the [\"Manage students\"]({url}) tab: "
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.
I think this is how to do it using dangerouslySetInnerHTML:
code-dot-org/apps/src/code-studio/components/ReportAbuseForm.jsx
Lines 125 to 132 in 1cad5ac
{/* we dangerouslySetInnerHTML because our string has html in it*/} | |
<div | |
dangerouslySetInnerHTML={{ | |
__html: i18n.t( | |
'project.abuse.report_abuse_form.abuse_type.question', | |
{ | |
link_start: '<a href="https://code.org/tos" target="_blank">', | |
link_end: '</a>' |
The corresponding string definition would look like this:
code-dot-org/dashboard/config/locales/en.yml
Line 884 in 40c5ac0
question: 'How does this content violate the %{link_start}Terms of Service%{link_end}?' |
except that this is formatted using dashboard string syntax, and you'd need apps string syntax.
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.
That looks correct to me! What happened when you tried 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.
@Hamms the string displayed as a string literal, like this:
"If your Code.org section is ever out of date with your list of students in Clever, click on the "Sync students from Clever" button on the ["Manage students"](/my/url/here) tab: "
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.
ah, it's the quotes. if i just have [Manage Students]({url})
instead of [\"Manage Students\"]({url})
, it works fine. the quotes aren't really necessary anymore, so i'm going to just delete them. thank you both for your help!
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.
Much better 👍 thanks for fixing!
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.
LGTM once the issue regarding html in translations is resolved.
Fixing bugs/nice-to-haves from our bug bash on teacher dashboard:
Link to "Manage Students" from /login_info, rather than just referencing it. Example:
For sections with 0 students, display an empty message on all tabs except "Manage Students" and the
/login_info
page:Default to "Progress" tab if no tab is chosen or the path does not match any teacher dashboard path.
Revert ordering and naming of tabs to original ordering/naming.