feat: refactor program dashboard to use react query#827
feat: refactor program dashboard to use react query#827asharma12-sonata wants to merge 66 commits intoopenedx:masterfrom
Conversation
our github actions should point to our release branch
chore: updating the workflows
…extra repositories (openedx#752)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Maxwell Frank <92897870+MaxFrank13@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…#783) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…edx#784) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
f11b964 to
2164115
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #827 +/- ##
==========================================
- Coverage 98.19% 98.19% -0.01%
==========================================
Files 145 150 +5
Lines 1388 1493 +105
Branches 298 322 +24
==========================================
+ Hits 1363 1466 +103
- Misses 25 27 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MaxFrank13
left a comment
There was a problem hiding this comment.
Looks good. I had a few questions and suggestions. Also, can we add more screenshots and description similar to the original PR?
MaxFrank13
left a comment
There was a problem hiding this comment.
Had some additional comments. Also, please include screenshots of Programs being rendered to the dashboard. Also, we should enhance the description to match what we had in the old PR
… for banner image which is updated in api endpoint
|
@arbrandes This is ready for your eyes whenever you get time! It's a big one, but most of it is derived from the previous PR: #751 |
|
Alright, will do, thanks! Looks like there's some commit titles that might need fixing up in the meantime. |
arbrandes
left a comment
There was a problem hiding this comment.
Looks great! I do have some minor suggestions inline, though.
| <LearnerDashboardHeader /> | ||
| <Routes> | ||
| <Route path="/" element={<PageWrap><App /></PageWrap>} /> | ||
| {getConfig().ENABLE_PROGRAM_DASHBOARD && ( |
There was a problem hiding this comment.
Can you please add the new variable to src/config/index.js? It's also common practice to add it to the .env files, in the way of documentation.
And while we're at it, mind adding some documentation about this new feature to the README?
| isLoading, | ||
| isError: errorState, | ||
| error, | ||
| } = useProgramsListData() as { |
There was a problem hiding this comment.
If you cast the return value of the hook, you're second-guessing Typescript. If we want a strong type for the data (which we do!) the typing should happen in the hook itself.
Parameterize useQuery in src/data/hooks/queryHooks.ts:37-42 with useQuery<ProgramData[]>({...}) (and give fetchProgramsListData a return type in api.ts). Then the consumer can destructure without an as cast and the runtime guards still make sense
There was a problem hiding this comment.
@arbrandes Implementing this change as suggested would move us away from the current pattern. I couldn’t find any similar implementations in the referenced files—please let me know if I missed something.
There was a problem hiding this comment.
There was no pattern in Learner Dashboard. It didn't use Typescript. We're setting the pattern with this, which makes it doubly important that we do it right. Please try doing it as I suggested.
There was a problem hiding this comment.
@arbrandes i have made these required changes, please have a look and let me know if there is anything else needed.
CC: @MaxFrank13
| import { logError } from '@edx/frontend-platform/logging'; | ||
|
|
||
| import appMessages from 'messages'; | ||
| import { useProgramsListData } from '../../../data/hooks/queryHooks'; |
There was a problem hiding this comment.
Probably best to follow the useInitializeLearnerHome pattern: expose useProgramsListData from the index barrel, and import instead from ../../../data/hooks.
This PR includes changes from
#751
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_DASHBOARD environment variable, not to be confused with the ENABLE_PROGRAMS variable, 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.jsx file had to be refactored. The LearnerDashboardHeader and FooterSlot were moved out of App.jsx and into index.jsx. This aligns with the way other MFEs are setup. The h1 tag for the app was also moved to the LearnerDashboardHeader so 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
Other changes are related to using react query for program dashboard.
Programs Dashboard