feat competency-selection tree, Course Search, and gradeable-subsection browse UI for Competency Criteria Associations - #3235
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ency management page
|
Thanks for the pull request, @AShatsila! 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. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 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 #3235 +/- ##
==========================================
+ Coverage 95.92% 96.02% +0.10%
==========================================
Files 1397 1414 +17
Lines 33581 34430 +849
Branches 7947 8191 +244
==========================================
+ Hits 32214 33063 +849
- Misses 1308 1311 +3
+ Partials 59 56 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thank you for the quick turnaround on this! I reviewed this PR using AShatsila/frontend-app-authoring@ashatsila/680-feat-competency-tree...ashatsila/670-competency-selection-tree while we wait for #3218 to merge. All of these items were identified by me, though I did have Claude draft this comment.
-
Indentation compounds across four separate places, producing much more gap than the design calls for.
CourseOutlineSubtree.tsx's outline wrapper carriesml-4 pl-2(32px), each section box then adds its ownml-3(16px) on top of that, the section box'spadding: 10px 8pxadds another 8px, andCourseSearchBrowse.scss's&__subsectionrule adds a furthermargin-left: 16pxon top of all of it. By the time you reach a subsection row that's roughly 70-80px of stacked indent from four independent sources instead of one consistent step per nesting level. Could you consolidate this to a single indent value applied once per level? -
Subsection rows overflow the right edge of their section box.
SubsectionRowrenders a Paragon<Button block ...>, andblocksetswidth: 100%on it.CourseSearchBrowse.scss's&__subsectionthen addsmargin-left: 16pxto that same element. A block element atwidth: 100%plus an additionalmargin-leftdoesn't shrink to compensate, it overflows its parent's right edge by exactly that 16px, which is visible in the screenshot in the PR description. Suggestwidth: calc(100% - 16px)alongside the margin, or moving to a padding-based indent on the parent instead of a margin on the child. -
Inline style at
CompetencyAssociationsPanel.tsx:29.<div className="d-flex" style={{ gap: '20px' }}>could use a Paragon/Bootstrap gap utility instead, the same PR already uses<Stack direction="horizontal" gap={3}>inCourseSearchBrowse.tsx, so there's already an established pattern for this in the same diff. -
Nit: duplicated predicate in
CourseOutlineSubtree.tsx:79and:88. BothhasGradedSubsectionandgradedSubsectionsrecompute the identicalsubsection.category === 'sequential' && subsection.gradedfilter separately. Could you compute each section's graded subsections once, then derivehasGradedSubsectionfrom whether any of those lists is non-empty? -
Hardcoded hex colors in
CourseSearchBrowse.scss.$group-border-color-course,-section,-subsection, and the#FFFFFFfills are literal values, while the same file uses Paragon tokens elsewhere (var(--pgn-color-light-400),var(--pgn-color-info-100)). Could these map to the closest Paragon neutral/gray tokens instead, for consistency with the rest of the file and for theming? -
No test coverage for
Resizable.tsx's drag behavior. This PR adds thehandleSidebranch to the drag math (onMouseMove's left-vs-right delta calculation), but there's no test file for this component at all, before or after this change. Could you add coverage that simulatesonMouseDown/onMouseMove/onMouseUpfor bothhandleSidevalues, including the min/max width clamping? -
The right-hand panel shouldn't render at all until a competency is selected. Right now
CourseSearchBrowse.tsx:191-197renders a "select a competency" prompt in place of the search field and course list, which means the panel itself, its background, its border, its column space, is still visible before anything is selected. We'd like the whole right-hand panel to not render at all in that state, not show a prompt inside a visible panel. This will need a change inCompetencyAssociationsPanel.tsxto only mountCourseSearchBrowseonceselectedCompetencyis set, rather than always mounting it and letting it branch internally onactiveCompetency. We're updating the ticket's AC to say this explicitly, flagging now so you're not blocked waiting on that ticket update to start on it. -
The course row's org/number/run line doesn't appear in the Figma design.
CourseRow.tsxrenders{course.org} / {course.number} / {course.run}as a subtitle under every course title. Checking this against the Figma screens, course rows there show only an icon and title, no subtitle line. Could you remove it, or let us know if there's a reason to keep it, for example disambiguating two courses with the same title, that we should account for in the design instead? -
Question, not a change request: does the "Courses & Content" container styling deviate from Figma on purpose? Figma shows each course row as its own separately-carded white box with a gap and shadow between rows, and "Courses & Content" as plain uppercase text with a rule beneath it, not a filled background. This PR instead wraps every row inside one shared white bordered container (
&__container), with "Courses & Content" as a filled, rounded-top colored strip (&__section-label). Was this an intentional simplification to better match how Paragon's own components typically group content, or should it be brought back in line with Figma's per-row cards and plain text label? -
All competency rows need to be selectable, not just leaves. This one needs to change: we're updating the ticket's AC so that clicking any competency row, group or leaf, selects it and opens the right-hand panel for it, the same behavior leaves already have. The expand/collapse arrow stays a separate control that only expands or collapses, it should not also select. Concretely,
CompetencyTreeItem.tsx'sisSelectable = !hasChildren && !!onSelectCompetencyneeds to drop the!hasChildrencondition so a group row gets the samerole="button"/aria-selected/click and keyboard handling a leaf row already gets. The tests inCompetencyTree.test.tsxthat currently assert a group row is never selectable (around lines 253 and 288) will need to be rewritten to assert the opposite. -
The subsection-click callback should come out of this PR.
CourseOutlineSubtree.tsxfiresonSubsectionSelected({usageKey, blockType})on a graded-subsection click, intended as plumbing for #672 to consume later. We checked this against #672's own technical spec: #672 actually adds its own separate select control to the rows this PR renders, and callsassociateSubsection(objectId, courseId), not by reusing a click on the whole row. That needscourseId, which this callback never carries, and it needs to be conditionally hidden per subsection based on the author's write access to that course, which a single always-on row click can't express either.blockType, the one extra field this callback does carry, isn't used anywhere in #672's spec. Given that mismatch, could you drop theonSubsectionSelectedprop and its wiring from this PR entirely? #672 will add its own control when it lands.
One more thing for your awareness, not a change request: the "Known limitation" note about the backend excluding null-start-date courses turns out to be moot, we've confirmed with the Open edX community that a course can never actually have a null start date. We'll get the ticket's AC updated to drop that line, no action needed on your end for it.
Description
Wires up the "Manage & Apply Competencies" page (
/taxonomy/:taxonomyId/competencies) with a course search/browse panel next to the existing competency tree, so a Platform Administrator can select a competency and find the course content to associate it with. Selecting a leaf competency in the left-hand tree activates a right-hand panel where they can search courses by title, filter by course-run start date, and expand a course to browse its gradeable subsections.This builds directly on top of the read-only competency tree page (closes openedx/openedx-core#680) and consumes the accessible-courses date-range filtering added on the backend in openedx/openedx-platform (openedx/openedx-core#669).
usageKey/blockTypeselection callback for a later ticket to wire into an actual "associate" action).Supporting information
Testing instructions
ENABLE_TAGGING_TAXONOMY_PAGESfeature flag./taxonomy/:taxonomyId/competencies.Other information
GET /api/contentstore/v2/home/coursesendpoint (now withstart_date_on_or_after/start_date_on_or_beforequery params from feat: add open managed group type #669) and the existingCourseIndexView/outline endpoint via the already-shippeduseCourseOutlineIndexhook.DatepickerControlcomponent wasn't viable as-is: that component only supports picking a single date, not a start/end range, so the date filter is built directly onreact-datepicker's own range mode (selectsRange) with a custom trigger button instead.DatepickerControl.jsxwas converted to.tsxin the process; its behavior is unchanged.ResizableBox(src/generic/resizable/) gained ahandleSideprop ('left'default, unchanged for existing callers such asSidebar.tsx;'right'used here) so the competency tree's resize handle can sit on its right edge, matching a panel on the left side of a layout instead of the right.CourseOverview.get_all_courses()) excludes courses with no start date whenever either date bound is set. The original ticket for feat: add open managed group type #669 explicitly calls for null-inclusive filtering ("Courses with no start date always appear, regardless of the range"), and the shipped implementation's own test (test_get_all_courses_by_start_date_excludes_no_start_date_course) asserts the opposite. This means a course without a start date will disappear from this panel's results as soon as a date filter is applied. Filed as a backend follow-up rather than fixed here since it touches already-merged code outside this repo.<button>s (native keyboard support); the competency tree's leaf rows userole="button"/tabIndex/aria-selectedwith Enter/Space handling since they're plain<div>s rather than native buttons, to keep the existing row styling.Best Practices Checklist
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks).apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src. (One exception:DatepickerControl.tsxkeeps the relative../../utils/../../constantsimports carried over from the.jsxfile it was renamed from — out of scope to touch here.)