Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #751 +/- ##
==========================================
- Coverage 97.54% 97.18% -0.37%
==========================================
Files 148 155 +7
Lines 1302 1419 +117
Branches 225 247 +22
==========================================
+ Hits 1270 1379 +109
- Misses 31 39 +8
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@openedx/openedx-product-managers this is ready for review |
|
@MaxFrank13, can you make codecov any happier? |
deborahgu
left a comment
There was a problem hiding this comment.
I'm approving based on our prior reviews of this feature branch, but obviously wait for product manager review before merging.
|
Note for @openedx/openedx-product-managers -- this is list page only. The details page is still legacy. |
|
GTG from a product perspective. From a frontend perspective I'd like to make sure we're not making liberal uses of overrides to Paragon styles (I haven't reviewed for this) - the more default the Paragon styling is, the more consistent the platform looks. |
There was a problem hiding this comment.
@sarina with regards to overrides to Paragon styles, this is the only CSS file in the changes. Everything else is styled using default Paragon components. Any modifications to them are done with utility classes from Paragon so there is no custom spacing or anything like that (e.g. padding: 10px; or whatever).
I had to add these two classes because Paragon's <Truncate/> component has been deprecated. Perhaps these classes are worthy additions to the base Paragon utilities, or perhaps someone has come up with another preferred solution. I can bring this up in this week's working group meeting.
There was a problem hiding this comment.
It'd be great if you could bring it up! Thank you 🙂
|
@arbrandes Do you have any thoughts on this? If merged, it will only change the out-of-the-box experience by changing the way the The Program Dashboard code is all behind the new flag |
|
@arbrandes Max will be out for a few weeks starting next Monday, so I'll be taking any comments or feedback whenever you get a chance to look at this! |
arbrandes
left a comment
There was a problem hiding this comment.
I'm generally in favor of the approach. I do have some inline questions before approving, though.
My biggest concern is tangential (i.e., it doesn't block this PR): I would prefer not introducing a new toggle if we can use plugin slots, but for new routes we can only do that cleanly with frontend-base. Would you consider submitting a parallel PR that implements the same feature but targetting the frontend-base branch?
It would not be a straightforward port because of the way the header and routing work in frontend-base-land. The only question is whether you submit the initial PR porting it, or if I do it. Either way we'd probably both be looking at it, at some point. :)
src/containers/ProgramDashboard/ProgramsList/ProgramListCard.tsx
Outdated
Show resolved
Hide resolved
| <> | ||
| <Helmet> | ||
| <title> | ||
| {formatMessage(messages.programDashboardPageTitle)} |
There was a problem hiding this comment.
A11y concern: this PR introduces an <h1> to the header for both the Learner Dashboard and the Programs pages, but here we're using Helmet to change it for Programs. Shouldn't the h1 be modified according to the page?
There was a problem hiding this comment.
Ah good call out!
here we're using Helmet to change it for Programs
We actually aren't changing the h1 here. We are changing the title for the page, which is different but it should match the theme of whatever the h1 is for the page. The h1 is set to "Learner Home" for both routes as you mentioned. The title for the CoursesPanel view is "Learner Home". But as you called out for the ProgramDashboard route, there it is being set to "Program Dashboard". So there is inconsistency here: we are modifying the title only for Program Dashboard route to be specific to Programs Dashboard but not doing the same for the default CoursesPanel view (App.jsx).
So I think it's a question of: do we want to modify the page title at all? And if so, should we also modify the h1 based on the route?
Or in other words: does "Learner Home" make sense as an h1 for both the CoursesPanel (App.jsx) view and the Programs Dashboard view? If so, then we shouldn't be modifying the page title in Helmet and keep it consistent across both routes. FWIW, we have been using h2 for the CoursesPanel tab (i.e. "My Courses") which would imply that we shouldn't change the h1 based on which tab we are in.
That's a lot of words. Totally down to discuss in a call if that's easier!
|
@arbrandes I've addressed all of the comments and made updates accordingly. The only one I haven't made is the one regarding the
Admittedly, I need to brush up on my |
Related Github Issue in platform roadmap
[Proposal] Legacy Program Dashboard conversion
This PR adds the programs dashboard in accordance with the above proposal. This is a conversion of the legacy programs dashboard that lives in edx-platform. This PR converts the legacy frontend into a React based frontend that lives under its own route. The route is conditionally rendered based on a new
ENABLE_PROGRAM_DASHBOARDenvironment variable, not to be confused with theENABLE_PROGRAMSvariable, which only handles the rendering of the "Programs" tab. This is done so that operators can choose to either use the legacy frontend or the new React-based frontend.In order to create a new route in this MFE, the
App.jsxfile had to be refactored. TheLearnerDashboardHeaderandFooterSlotwere moved out ofApp.jsxand intoindex.jsx. This aligns with the way other MFEs are setup. Theh1tag for the app was also moved to theLearnerDashboardHeaderso that it would appear on all routes. The Header logic has also been refactored so that the correct tab is highlighted based on the pathname of the page.All other changes are related to the Program Dashboard itself. The dashboard uses the
/api/dashboard/v0/programs/endpoint to retrieve enrollment data.Directory structure
Programs Dashboard
Has enrollments
Does not have enrollments
Error retrieving enrollment data