-
Notifications
You must be signed in to change notification settings - Fork 164
feat: add course import page [FC-0112] #2580
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 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. Where 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
src/library-authoring/import-course/CourseImportHomePage.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/import-course/CourseImportHomePage.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/import-course/CourseImportHomePage.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/import-course/MigratedCourseCard.test.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/import-course/MigratedCourseCard.test.tsx
Outdated
Show resolved
Hide resolved
|
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido Nice work! This looks good. Left some suggestions.
src/library-authoring/data/api.ts
Outdated
| await getAuthenticatedHttpClient().post(getLibraryContainerPublishApiUrl(containerId)); | ||
| } | ||
|
|
||
| export interface CourseMigration { |
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.
NIT: rename to indicate import instead of migration
| migrations: (libraryId: string) => [ | ||
| ...libraryAuthoringQueryKeys.contentLibrary(libraryId), | ||
| 'migrations', |
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.
Same here. Rename to import.
| title={libraryData.title} | ||
| org={libraryData.org} | ||
| contextId={libraryId} | ||
| isLibrary |
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.
Pass readOnly from library context here.
| isLibrary | |
| isLibrary | |
| readOnly={readOnly} |
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.
Thanks! Fixed here: f17820b
| .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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this completely. It unnecessarily adds vertical scroll
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <div className="course-migration-help pt-3 border-left"> | |
| <div className="pt-3 border-left"> |
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.
Fixed cab000a
| /> | ||
| </span> | ||
| </Stack> | ||
| <hr /> |
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.
| <hr /> | |
| <hr className="w-100" /> |
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.
Fixed: cab000a
| size: undefined, | ||
| }} | ||
| /> | ||
| <Container className="px-0 mt-4 mb-5 library-authoring-page"> |
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.
| <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
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.
Thanks! Fixed: ab24e7c
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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