Repository navigation
[Remove Vuetify from Studio] Add StudioContentNodeIcon to channel details Total resources (#5933) - #6379
[Remove Vuetify from Studio] Add StudioContentNodeIcon to channel details Total resources (#5933)#6379vtushar06 wants to merge 1 commit into
Conversation
|
👋 Hi @vtushar06, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
📢✨ Before we assign a reviewer:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
🟡 Waiting for changesOutcome: reviewed Last updated: 2026-11-03 05:53 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6379: H5P rows lose their icon, and the PR has no screenshots. The component and color helper otherwise follow the spec.
CI passing. Tests not run locally. Manual QA did not run.
- blocking: H5P rows render without an icon (inline).
- blocking: Screenshots are missing, and the AC requires them. The issue also asks for a PDF export check. Add before/after screenshots of the modal and the generated PDF, in LTR and RTL.
- suggestion: Chip size may differ from base (inline).
- suggestion: Tests would still pass if the swap regressed (inline).
- nitpick: Docstring restates the code (inline).
Next step: Needs a full code review by a human engineer, then manual QA once that reviewer sets Needs QA.
- The author is outside the team: confirm this should be built at all.
- User-visible files changed:
contentcuration/contentcuration/frontend/shared/utils/colors.js,contentcuration/contentcuration/frontend/shared/views/StudioContentNodeIcon.vue,contentcuration/contentcuration/frontend/shared/views/details/StudioDetailsPanel.vue. - Choose the H5P icon; the issue left it open.
- Compare the modal chips and PDF export against base in LTR and RTL.
Written by rtibblesbot, an LLM-based coding agent.
| icon() { | ||
| try { | ||
| return getContentKindIcon(this.kind, this.isEmpty); | ||
| } catch (e) { |
There was a problem hiding this comment.
blocking: H5P rows lose their icon, and this catch hides it.
shared/utils/icons.jshas noh5pentry, so the lookup throws.- This catch returns
null, so the H5P row in Total resources shows a label with no icon. - Base falls back to
'error_outline'(shared/vuetify/icons.js:28). - The AC rules out visual changes beyond the listed ones.
- The
H5Pentry ingetContentKindColoris dead code as a result.
Give h5p an icon, then drop the catch so a missing mapping fails loudly. html5 fits, since the label already says "HTML5 App".
| :style="{ backgroundColor: backgroundColor }" | ||
| capture-as-image | ||
| > | ||
| <KIcon |
There was a problem hiding this comment.
suggestion: This chip's size likely differs from base. Base is a Vuetify small chip with 22px content height and 0 5px padding. Here KIcon uses its default size with 2px 5px padding. Set an explicit size, or show in screenshots that it matches.
| }, | ||
| loading: false, | ||
| }); | ||
| expect(screen.getByText('Video')).toBeInTheDocument(); |
There was a problem hiding this comment.
suggestion: This test doesn't cover the icon swap: it passes unchanged on base. '30'/'12' can also match any text on the page. Scope each row with within(...), assert the icon renders, and add an h5p row.
| 'renders an icon for %s', | ||
| kind => { | ||
| const { container } = renderComponent({ kind }); | ||
| expect(container.querySelector('svg')).toBeInTheDocument(); |
There was a problem hiding this comment.
suggestion: This test can't catch a wrong icon or background: it only checks that an svg exists. Assert the background from getContentKindColor and something icon-specific per kind. Add h5p to the list.
| import { ContentKindsNames } from 'shared/leUtils/ContentKinds'; | ||
|
|
||
| /** | ||
| * Returns the KDS background color for a content node kind. |
There was a problem hiding this comment.
nitpick: This docstring restates the code. The summary repeats the function name. The fallback line repeats line 25. @param/@returns add nothing the keys on lines 15–23 don't show. Delete lines 4–10.
Summary
Removes the Vuetify dependency from the content kind icons in the channel details modal (Total resources section) by adding a Vuetify-free
StudioContentNodeIcon, as a parallel toContentNodeIcon.Changes:
getContentKindColorinshared/utils/colors.js, mapping content kinds to KDS theme colors (tokens.watch/listen/read/practice/explore, andpalette.grey.v_400for folders and unknown kinds)StudioContentNodeIcon(shared/views/StudioContentNodeIcon.vue), built onKIcon+getContentKindIconfromshared/utils/icons.js. Keepscapture-as-imageso the PDF channel summary still rasterises the iconsContentNodeIcon→StudioContentNodeIconinStudioDetailsPanelTotal resources onlyContentNodeIconandshared/vuetify/theme.jsare untouched; no other areas refactoredNotes / open questions:
shared/utils/icons.jshas noh5p(orunsupported) mapping and throws for them, soStudioContentNodeIconrenders nothing for unmapped kinds instead of breaking the modal. Happy to maph5p→html5if preferredkindandisEmptyprops are carried over;includeText,showColor,fillWidthandsmallaren't used in the details panel so I left them out to keep scope tight. Can add them if wanted for future sub-issuesScreenshots:
Published Channel, dev server,
[email protected]. Background colours match on both sides (exercise#6986f7, audio#ff7f8d, document#ff4324, HTML5#ff7600, video#33acf5). The after chips use KDS icons at a 4px radius instead of Vuetify Material chips at 2px; the kinds, labels, and counts are the same. The clipped Arabic PDF layout is already present onunstable.unstable)remove-vuetify-5933)References
Closes #5933
Sub-issue of #5060
Reviewer guidance
pnpm test colors StudioContentNodeIcon StudioDetailsPanel— 30 tests pass[email protected]/a→ Channels > My Channels → info icon on the Published Channel card → check the Total resources iconsAI usage
I used an AI assistant to help draft the helper, component and tests, following the existing
ContentNodeIcon/StudioDetailsPanelpatterns and the issue's scope. I reviewed the generated code, kept the change limited to the files in the issue, and ran the tests and linting locally.