P20-119: Add pre-lockdown parental permission banner #58434
Conversation
…e display the modal
…t-permission-modal
…t-permission-modal
apps/src/templates/Notification.jsx
Outdated
| const colorStyles = styles.colors[type]; | ||
| const colorStyles = color | ||
| ? {backgroundColor: color, borderColor: color} | ||
| : styles.colors[type]; |
There was a problem hiding this comment.
Since type is now not required, but either color or type seem to need to exist... and since Notification isn't TypeScript and can't support a discriminated union for its properties... maybe we need to at least raise some error if neither key is given. Or have a default type?
There was a problem hiding this comment.
I'm also wondering if it would be more consistent to add your own type instead of adding the ability to directly manipulate the color so that updating the styling the notifications at-large can be made consistent.
There was a problem hiding this comment.
I agree with your point about focusing on consistency. However, I bilieve that creating a custom type for each minor change could lead to a bloated component over time. Directly managing styles might be a more efficient approach, ensuring the component stays streamlined and its purpose remains clear
There was a problem hiding this comment.
Yep, that makes sense. Leave it as you had it.
| }; | ||
|
|
||
| export const UpdateRequestForm = () => { | ||
| const state = { | ||
| parentalPermissionRequest: { | ||
| parent_email: 'parent@email.com', | ||
| requested_at: new Date().toISOString(), | ||
| requested_at: Date(), |
There was a problem hiding this comment.
Seems so haphazard to trust the localized string of the date instead of something more consistent. Is this really the way one is supposed to handle dates? I'm sure moment can magically handle anything but it feels so wrong.
There was a problem hiding this comment.
I'll revert this change
There was a problem hiding this comment.
I appreciate your effort on this. I'm never quite confident about handling date serialization. I normally default to the iso format whenever there is doubt, but I'm still ready to be convinced otherwise.
There was a problem hiding this comment.
When working with an API, the date must be in ISO format. However, I think this is not as crucial for a storybook example
apps/src/sites/studio/pages/policy_compliance/parental_permission/_modal.js
Outdated
Show resolved
Hide resolved
apps/src/templates/policy_compliance/ParentalPermissionBanner/index.tsx
Outdated
Show resolved
Hide resolved
| @@ -80,6 +81,12 @@ function showHomepage() { | |||
| } | |||
|
|
|||
| const announcement = getTeacherAnnouncement(announcementOverride); | |||
| const parentalPermissionBanner = homepageData.parentalPermissionBanner && ( | |||
| <ParentalPermissionBanner | |||
| key="parentla-permission-bnner" | |||
There was a problem hiding this comment.
two subtle typos in the key: parental-permission-banner
| const inSection = getScriptData('inSection'); | ||
|
|
||
| const reportEvent = (eventName, payload = {}) => { | ||
| payload.isSection = inSection; |
There was a problem hiding this comment.
is this supposed to be isSection or inSection in the event?
| @@ -6,7 +6,8 @@ | |||
| - scriptOverviewData = {scriptData: @script_data, plcBreadcrumb: @plc_breadcrumb} | |||
|
|
|||
| - content_for(:head) do | |||
| %script{ src: webpack_asset_path('js/scripts/show.js'), data: {scriptoverview: scriptOverviewData.to_json }} | |||
| %script{src: webpack_asset_path('js/scripts/show.js'), | |||
| data: {scriptoverview: scriptOverviewData.to_json, parental_permission_banner: current_user && parental_permission_banner_data(current_user, request).to_json}} | |||
There was a problem hiding this comment.
Technically, this sets the field to "null" when there is no banner data... and that does work since it is parsed in show.js. The logic in show.js seems to support this dataset field being actual null or undefined. Could we have the data field not exist in that case?
Summary
The banner will appear for students who are not compliant with the Child Account Policy while the CPA experience is
cpa_all_user_lockout_warning.Note: Use the parameter
?show_parental_permission_bannerto force the display of the banner.Screenshots
Home Page
Course Page
Account Page
Links