-
Notifications
You must be signed in to change notification settings - Fork 80
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: create library form [FC-0059] #1116
feat: create library form [FC-0059] #1116
Conversation
Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
1b451bb
to
a82a7bc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1116 +/- ##
==========================================
- Coverage 92.68% 92.68% -0.01%
==========================================
Files 693 693
Lines 12347 12346 -1
Branches 2661 2661
==========================================
- Hits 11444 11443 -1
Misses 872 872
Partials 31 31 ☔ View full report in Codecov by Sentry. |
bd7e63f
to
8724ac0
Compare
c6f08df
to
a008837
Compare
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 looks and works great @rpenido !
Couple of small nits: Will let Braden weigh in on the description
field issue (discussion on the open-craft PR), and I've raised a todo for later with the backend error reporting.
- I tested this on my tutor dev (nightly) stack using the PR test instructions
- I read through the code (using feat: create library form (TEMP) open-craft/frontend-app-authoring#42)
- I checked for accessibility issues by using my keyboard only to create a library
- Includes documentation -- help text for form fields
- User-facing strings are extracted for translation
setApiError(( | ||
<> | ||
{error.message} | ||
<br /> | ||
{JSON.stringify(error.response?.data)} | ||
</> | ||
)); |
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 What's the status of this PR - is something blocking the review? |
@bradenmacdonald This PR depends on this one: If you want to do a pass (that would be awesome), it would be better to use the following temp PR for the diff: open-craft#42 |
@rpenido Sure, I did a quick pass on the temp PR. It looks good; just had some minor comments about react query. |
4d9da7f
to
63e7593
Compare
@rpenido Please rebase this and get the coverage test passing; I can review whenever you're ready. |
8466521
to
f07b0bd
Compare
Hi @bradenmacdonald! This is ready for your review. |
@rpenido Is this form for V1 libraries or V2 libraries? |
V2 libraries |
c539118
to
af3a9d1
Compare
af3a9d1
to
8fa7880
Compare
@KristinAoki FYI, You will see this on your devstack because we have it enabled by default for developers, but it's off by default in production, so this new library v2 UX that we're building shouldn't appear on edx.org. There, users should just see the existing "v1" UI which links to the legacy Studio. |
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 great, thanks @rpenido ! Just some minor suggestions before I approve + merge.
👍
- I tested this: on my tutor devstack
- I read through the code
- I checked for accessibility issues
- Includes documentation: n/a
return useMutation({ | ||
mutationFn: createLibraryV2, | ||
onSettled: () => { | ||
queryClient.invalidateQueries({ queryKey: ['listV2Libraries'] }); |
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 should use a "query key factory" for the query key instead of hard-coding a string here - see this example: https://github.com/openedx/frontend-app-course-authoring/blob/5dee203401c323b89492e3597934377284eb074d/src/taxonomy/data/apiHooks.js#L20-L66
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.
Done: d94bbe2
This included moving some api/apiHook functions to library-authoring
Co-authored-by: Braden MacDonald <[email protected]>
752b094
to
d94bbe2
Compare
Hi @bradenmacdonald! This is ready for another pass. |
@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR adds a Create Library form to course-authoring.
More Info
Testing Instructions
Private ref: FAL-3768