feat: add course import page [FC-0112]#2580
Conversation
|
Thanks for the pull request, @rpenido! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2580 +/- ##
========================================
Coverage 94.82% 94.83%
========================================
Files 1226 1231 +5
Lines 27557 27627 +70
Branches 6043 6221 +178
========================================
+ Hits 26131 26199 +68
- Misses 1368 1370 +2
Partials 58 58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bb4eff0 to
9230c0e
Compare
9230c0e to
ca3c068
Compare
|
@rpenido The right chevron is missing here.
Also, the collection link color and underline is not matching with designs. |
navinkarkera
left a comment
There was a problem hiding this comment.
@rpenido Nice work! This looks good. Left some suggestions.
| await getAuthenticatedHttpClient().post(getLibraryContainerPublishApiUrl(containerId)); | ||
| } | ||
|
|
||
| export interface CourseMigration { |
There was a problem hiding this comment.
NIT: rename to indicate import instead of migration
| migrations: (libraryId: string) => [ | ||
| ...libraryAuthoringQueryKeys.contentLibrary(libraryId), | ||
| 'migrations', |
There was a problem hiding this comment.
Same here. Rename to import.
| title={libraryData.title} | ||
| org={libraryData.org} | ||
| contextId={libraryId} | ||
| isLibrary |
There was a problem hiding this comment.
Pass readOnly from library context here.
| isLibrary | |
| isLibrary | |
| readOnly={readOnly} |
| .course-migration-help { | ||
| z-index: 1000; // same as header | ||
| flex: 350px 0 0; | ||
| position: sticky; | ||
| top: 0; | ||
| right: 0; | ||
| height: 100vh; | ||
| overflow-y: auto; | ||
|
|
||
| hr { | ||
| width: 100%; | ||
| } | ||
| } |
There was a problem hiding this comment.
We can remove this completely. It unnecessarily adds vertical scroll
There was a problem hiding this comment.
I was trying to avoid the same issue from here: #2200 (comment)
Removed: cab000a
| import messages from './messages'; | ||
|
|
||
| export const HelpSidebar = () => ( | ||
| <div className="course-migration-help pt-3 border-left"> |
There was a problem hiding this comment.
| <div className="course-migration-help pt-3 border-left"> | |
| <div className="pt-3 border-left"> |
| /> | ||
| </span> | ||
| </Stack> | ||
| <hr /> |
There was a problem hiding this comment.
| <hr /> | |
| <hr className="w-100" /> |
| size: undefined, | ||
| }} | ||
| /> | ||
| <Container className="px-0 mt-4 mb-5 library-authoring-page"> |
There was a problem hiding this comment.
| <Container className="px-0 mt-4 mb-5 library-authoring-page"> | |
| <Container className="mt-4 mb-5"> |
px-0 adds unnecessary horizontal scroll and library-authoring-page has no effect
e11f76f to
ab24e7c
Compare
I missed that! Thanks!
I was trying to stick to our default Paragon design (https://paragon-openedx.netlify.app/components/button/#when-to-use-the-inline-size) and prevent custom styles. Both fixed here: 73948ab |
ChrisChV
left a comment
There was a problem hiding this comment.
The code looks good! @rpenido Thanks! 👍 I will merge it after fixing the broken lint and after the review of @navinkarkera
fb1eded to
fc79cb1
Compare
navinkarkera
left a comment
There was a problem hiding this comment.
@rpenido Great work 👍
- I tested this: (Played around with import home page)
- I read through the code
- I checked for accessibility issues
- Includes documentation
| <Link | ||
| to={`/course/${courseImport.source.key}`} | ||
| aria-label={intl.formatMessage(messages.courseImportNavigateAlt)} | ||
| className="text-primary-500" | ||
| > | ||
| <Icon src={ArrowForwardIos} /> | ||
| </Link> |
There was a problem hiding this comment.
This can be updated as part of #2526. The link should take user to import details page and I think the whole card should be clickable.
Co-authored-by: Navin Karkera <[email protected]>
|
@rpenido is it ready to be merged? |
Yeah! |

Description
This PR adds the Library Import Home, which lists course migrations to the library
"Course Author" and "Developer"
Supporting information
Testing instructions
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypes,defaultProps, andinjectIntlpatterns are not used in any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'Private ref: FAL-4265