-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: add cookies for archived courses in highlights #1160
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1160 +/- ##
==========================================
+ Coverage 85.44% 85.46% +0.02%
==========================================
Files 497 498 +1
Lines 10756 10830 +74
Branches 2256 2267 +11
==========================================
+ Hits 9190 9256 +66
- Misses 1523 1530 +7
- Partials 43 44 +1 ☔ View full report in Codecov by Sentry. |
src/components/ContentHighlights/ContentHighlightArchivedCoursesAlert.jsx
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/ContentHighlightArchivedCoursesAlert.jsx
Outdated
Show resolved
Hide resolved
src/components/EnterpriseApp/data/hooks/useEnterpriseCuration.js
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/ContentHighlightArchivedCoursesAlert.jsx
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/ContentHighlightArchivedContentAlert.jsx
Outdated
Show resolved
Hide resolved
* the content is not considered archived. An "unpublished" course run is not considered "archived" because | ||
* it could have a scheduled course run in the future. |
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.
[clarification] I suppose the reason I'm concerned about unpublished
is that a course that might have courseRunStatuses: ["archived", "unpublished"]
, the unpublished course run is effectively unusable by learners (hidden). The only actionable course run status in the courseRunStatuses
array remaining that's relevant / displayed to learners would be archived
, despite having a courseRunStatuses
length of 2.
With the logic, as is, in isArchivedContent
, we would consider the course to be non-archived despite the only available course run displayed to learners being archived.
src/components/ContentHighlights/ContentHighlightArchivedAlert.jsx
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/ContentHighlightArchivedAlert.jsx
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/ContentHighlightArchivedAlert.jsx
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/CurrentContentHighlightHeader.jsx
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/ContentHighlightArchivedAlert.jsx
Outdated
Show resolved
Hide resolved
src/components/ContentHighlights/ContentHighlightArchivedAlert.jsx
Outdated
Show resolved
Hide resolved
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 just left one suggestion for a proposed refactor to make logic a bit more concise in the function that determines whether the provided course run statuses are deemed "archived".
Description
Set cookies in the admin portal to indicate whether the admin's created highlights have newly archived courses. The goal is to display a red notification bubble on the 'Highlights' menu item in the sidebar whenever a course in their highlights is archived. This notification will persist until the user opens the Highlights summary page and dismisses the alert.
UI Changes
Screen.Recording.2024-02-08.at.2.50.41.PM.mov
JIRA
For all changes
Only if submitting a visual change