-
Notifications
You must be signed in to change notification settings - Fork 132
Add subsection/unit to section/subsection full-page view within library [FC-0090] #2152
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
Conversation
to make it easier to show or hide the "add new component" buttons. Also removes the need to convert between Chapter => Section, Sequential => Subsection, and Vertical => Unit.
instead of just opening the sidebar, when adding a container to another container. This saves the user a click and makes this consistent with the "add new component" button on unit page.
Thanks for the pull request, @pomegranited! 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2152 +/- ##
========================================
Coverage 94.12% 94.13%
========================================
Files 1164 1164
Lines 24527 24540 +13
Branches 5339 5220 -119
========================================
+ Hits 23086 23100 +14
- Misses 1364 1372 +8
+ Partials 77 68 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
expect(await screen.findByRole('button', { name: /existing library content/i })).toBeInTheDocument(); | ||
expect(await screen.findByRole('button', { name: /text/i })).toBeInTheDocument(); | ||
expect(await screen.findByRole('button', { name: /problem/i })).toBeInTheDocument(); | ||
}); | ||
}); |
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.
Also need to remove the testIf
skipped tests from ContainerInfo.test;
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.
Ah my mistake -- we can reinstate one of them, but the other relies on https://github.com/openedx/frontend-app-authoring/pull/2145/files#r2155581898
cf ec74917
Is there an issue to track that somewhere? I did notice this when testing. |
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.
👍 Seems good! Please just rebase / merge master and make sure it's working again before we can merge it. Also let me know about my minor comment and the testIf
thing you mentioned.
- I tested this: as suggested
- I read through the code
- I checked for accessibility issues
- Includes documentation
@@ -37,7 +37,7 @@ import { ContainerType } from '../../generic/key-utils'; | |||
|
|||
type ContentType = { | |||
name: string, | |||
disabled: boolean, | |||
disabled?: boolean, | |||
icon?: React.ComponentType, |
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.
icon?: React.ComponentType, |
Can we delete this icon
field now? Seems to be unused after these changes.
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.
Ah, I made a mistake here -- we do need that icon
parameter so we can show the little sparkle next to "Advanced / Other":
Fixed with 2e50209.
No, but I asked to Product for the exact feature |
d4ffcf9
to
2e50209
Compare
@pomegranited Looks good! I found some issues: On the section page, we need to change the first button to On the select subsection modal, we need to remove the |
to make it easier to type these variables in other places
When adding units to a subsection: * button names are "Add Unit" and "Existing Unit" * modal title and text is "Select units" and "N units selected" * modal button is "Add to subsection" When adding subsection to a section: * button names are "Add Subsection" and "Existing Subsection" * modal title and text is "Select subsections" and "N subsections selected" * modal button is "Add to section" Otherwise, the more generic "component" text is used.
Done with 3c62925. I also updated the "Select components" and "N components selected" text to be consistent with the context.
So, this would be harder to do, so I haven't done it. Since the search modal isn't aware of the library routing context, we'd have to add something to the LibraryContext to pass this context through, like we do with |
Thanks! Works great!
@pomegranited What do you think about adding |
Ah, turned out to be even easier than that :) 50aedd2 |
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 good 👍 Please, fix the CI to merge the PR
@ChrisChV Done :) Tests will pass now. |
Description
Affects Library Content Authors.
Supporting information
Testing instructions
Code review: Recommend working through commit by commit.
Ensure that the new Subsection is shown, and that its breadcrumbs reflect that it was added the Section
Ensure that the new Unit is shown. Navigate back to the Subsection to see that the Unit was added to the Subsection (see notes below).
Other information