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
Emergency mode banner for teacher dashboard #43702
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!
I think it might be good to note either in the PR notes or in a comment somewhere that while we only show this banner when postMilestoneDisabled
is true, that does not correspond directly to when a teacher might notice missing progress in the teacher dashboard. A teacher might notice at a later date that progress is missing for work that a student did while emergency mode was active. Side note: If we ever enable emergency mode, we may want to consider shipping a banner indicating when progress might have been lost for the week or two after the incident.
@@ -618,6 +618,8 @@ | |||
"disabledProgress1": "Yikes! This week is Hour of Code and we are experiencing even more traffic than we anticipated.", | |||
"disabledProgress2": "Unfortunately, we are not able to save the progress that you make on your course while we are dealing with this issue. Don't worry, your progress from before this week is still safe.", | |||
"disabledProgress3": "You can still try Hour of Code tutorials even though your progress won't save.", | |||
"disabledProgressTeacherDashboard1": "Warning: Progress not saving", | |||
"disabledProgressTeacherDashboard2": "Unfortunately, we are not able to save the progress that your students make on their course while we are dealing with this issue. Don't worry, their progress from before this week is still safe. They can still try Hour of Code tutorials even though their progress won't save.", |
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.
(minor) I thought "while we are dealing with this issue" sounded a bit negative but I see that this string is derived from the one above so am ok with just leaving this as-is.
@@ -70,6 +71,30 @@ class TeacherDashboardHeader extends React.Component { | |||
/> | |||
)); | |||
|
|||
progressNotSavingNotification() { | |||
let details = ( |
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.
Can this be const
?
@@ -304,6 +304,8 @@ def summarize(include_students: true) | |||
restrict_section: restrict_section, | |||
code_review_enabled: code_review_enabled?, | |||
is_assigned_csa: assigned_csa?, | |||
# this will be true when we are in emergency mode and this section has any script assigned other than CSP or CSD |
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.
It might be more accurate to say that this will be true for CSF and HoC courses? (Even more precisely, it is the set of scripts returned 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.
+1 to this suggestion because making the comment more specific will be more future-proof
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.
Thank you! This is definitely better!
<Notification | ||
type={NotificationType.failure} | ||
notice={i18n.disabledProgressTeacherDashboard1()} | ||
details={details} |
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 don't think I have a strong opinion either way, but is there a reason to do the "Learn more" as an inline link to the article rather than a button? Mostly asking since the Notification
component comes with the option to use a button built in, which to my memory is more commonly used.
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 for the tip - I switched it to use the detailsLink
props because the button looked weird with such a long notification text.
Emergency mode banner for teacher dashboard
LP-2082
Provide teachers with a warning similar to what we show students when we are in emergency mode. This banner will appear on the teacher dashboard when we are in emergency mode if the section they are viewing is assigned a script in the lists returned here.
Note: this banner will be visible while we are actually in emergency mode.If this happens, there will be a blog post explaining what happened and the times and scripts affected so that teachers who don't look at the teacher dashboard while this message is up will be able to see what happened. We could consider adding a separate banner after the fact alerting them and directing them to the blog post.
Links
jira ticket: LP-2082
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: