[Remove Vuetify from Studio] Buttons, dropdowns, and dialog in channel editor root page #5668
[Remove Vuetify from Studio] Buttons, dropdowns, and dialog in channel editor root page #5668vtushar06 wants to merge 6 commits into
Conversation
|
👋 Thanks for contributing! We will assign a reviewer within the next two weeks. In the meantime, please ensure that:
We'll be in touch! 😊 |
|
hi @MisRob, On small screens, the Edit channel option goes into the three-dots menu. The original code showed a red error icon when the channel has no language set. Since KDropdownMenu only accepts strings for option labels, I am thinking to use an emoji instead: let label = this.$tr('editChannel');
if (!this.currentChannel.language) {
label = `${label} ⚠️`;
}Is this okay, or would you prefer something like "Edit channel (incomplete)"? |
|
one more thing I wanted to ask about original code had some duplication where users could access the same thing from both the Share dropdown and three-dots menu:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the channel editor toolbar from Vuetify to KDS (Kolibri Design System) components as part of the larger effort to remove Vuetify from Studio. The changes replace VTooltip, VBtn, VList, and BaseMenu with their KDS equivalents (KTooltip, KButton, KIconButton, and KDropdownMenu).
Changes:
- Replaced Vuetify tooltips, buttons, and dropdown menus with KDS components
- Refactored menu logic into computed properties (
shareMenuOptions,channelMenuOptions) and handler methods (handleShareMenuSelect,handleChannelMenuSelect) - Updated tests to verify computed properties instead of DOM manipulation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| contentcuration/contentcuration/frontend/channelEdit/views/TreeView/TreeViewBase.vue | Migrated toolbar buttons, dropdowns, and tooltips from Vuetify to KDS components; added computed properties for menu options and handler methods for menu selections |
| contentcuration/contentcuration/frontend/channelEdit/views/TreeView/tests/TreeViewBase.spec.js | Updated tests to check computed properties directly instead of testing DOM elements; removed obsolete component stubs for BaseMenu and MessageDialog |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @vtushar06,
Emoji sounds creative ;)!
Yes, as you mentioned, let's keep the options exactly the same now. I am not sure what was the original decision, it may be intentional - and not exactly in scope of this project. |
|
Before we assign a maintainer, I will first try inviting the community review ;) 📢 ✨ Community Review guidance for both authors and reviewers. |
|
Hii @MisRob! Thank you,
...
I suggest keeping the emoji approach because the warning appears after the text, which is more natural for an indicator |
|
Hey @vtushar06 ! I am still learning testing , I see the tests now check the computed properties directly |
|
hey @abhiraj75, In our codebase, we're still migrating from old test patterns. The TreeViewBase tests use wrapper.vm computed properties, which is what was approved as not in this PR scope. But best practice going forward is to test user-observable behavior (what appears on screen) rather than internal handler calls. |
|
Thanks @vtushar06 ! |
|
Hi @vtushar06
Thanks for following-up, appreciated! And meanwhile similar issues showed up in other places. I'm now coordinating a decision on how to proceed. Will let you know soon. |
|
@vtushar06 we will update KDS to allow for easier dropdown item customization. Tracking here learningequality/kolibri-design-system#1200. As soon as it's done I will install the new KDS version in Studio, and then you can use it :) In any case, thanks for raising that and also thinking through options. I prefer KDS update because it will be useful in more places and in general, and will allow us to use KDS icons as usual. |
|
sure @MisRob, please let me know If I can help there too, I would like to work on that issue in KDS.. |
Gauravchy09
left a comment
Gauravchy09
left a comment
There was a problem hiding this comment.
Hey @vtushar06 , thanks for the ping! I took a pass through the changes — overall this looks like a solid cleanup.
A couple of small things I noticed:
Trash link: In handleChannelMenuSelect, we’re toggling showBinModal. The earlier version was routing to the trash page (trashLink). Just checking — was the move to a modal intentional?
Vuetify remnants: I still see in the template and a $vuetify.breakpoint check in the script. Might be worth swapping those out so the page is fully off Vuetify.
Menu duplication: The sharing items (submit to library, etc.) appear in both shareMenuOptions and channelMenuOptions. Maybe we can extract those into a shared helper to avoid duplication.
The KDS dropdown integration itself looks clean. Happy to dig deeper if needed.
|
Hey @Gauravchy09, thanks for taking the time to review really appreciate it |
|
Thanks both. As for Vuetify breakpoints ~ for now no problem to keep |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean migration from Vuetify to KDS for the channel editor toolbar.
CI passing. Screenshots reviewed — toolbar, share dropdown, three-dots menu, mobile collapse, and delete modal all look correct.
Acceptance criteria for #5597:
-
Specification followed (VBtn, VList, BaseMenu, VTooltip replaced with KDS equivalents)
-
$vuetify.breakpointusage preserved -
No
::v-deepor/deep/selectors -
PR includes screenshots
-
Share and Publish collapse into three-dots on small screens
-
No visual differences — see blocking finding (mobile error indicator lost)
-
Strings translated properly
-
notranslateclass present where needed -
Unit test suite meaningfully updated
-
blocking (1): No-op code drops the mobile error indicator for missing channel language — see inline comment
-
suggestion (1): Tests don't cover
channelMenuOptions,handleChannelMenuSelect, orhandleShareMenuSelect -
praise (2): See inline comments
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
|
Hi @vtushar06, we will soon release the KDS version with the update you prepared & then we can start wrapping it up here. |
|
Would you be able to have a look at the conflicting file? |
|
yeah sure @MisRob.I will look into it. |
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Thanks a lot @vtushar06! Looks pretty well! Just a few thoughts :).
|
Hi, the latest KDS is now merged to |
|
Hey @MisRob @AlexVelezLl,I have tested it at the live examples- same behaviour there too. After pressing Enter on any option, focus always goes back to the first item regardless of which one was selected. So it looks like this is a KDS-level thing, not something specific to our Studio code. In Studio, on top of that, focus disappears completely when the modal or panel opens after selection - probably because the panel/modal grabs focus away. But the root focus issue is visible on the docs page in isolation. |
vtushar06
force-pushed
the
remove-vuetify-5597
branch
from
21c8cdb to
9c4872f
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Prior findings
5 prior findings stable (4 resolved, 1 acknowledged — unchanged since last review).
Newly resolved:
- Missing analytics in PUBLISH handler (suggestion, review 3) ← commit 9c4872f replaced
this.showPublishSidePanel = truewiththis.publishChannel()
Still unaddressed (re-raised below):
- Test coverage for
channelMenuOptions,handleChannelMenuSelect,handleShareMenuSelect(suggestion, first raised in round 1) — no reply, no tests added across 3 rounds
6/7 prior findings resolved or acknowledged. 1 re-raised below.
CI passing. Screenshots reviewed — desktop toolbar (SHARE + PUBLISH + three-dots), mobile collapse, share dropdown, delete modal, and three-dots menu in both viewports all look correct.
- suggestion (1): Missing test coverage for menu options and handlers — see inline
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| expect(menuOptions.length).toBe(3); | ||
| expect(menuOptions.some(item => item.value === 'submit-to-library')).toBe(true); | ||
| expect(menuOptions.some(item => item.value === 'invite-collaborators')).toBe(true); | ||
| expect(menuOptions.some(item => item.value === 'share-token')).toBe(true); |
There was a problem hiding this comment.
suggestion: The channelMenuOptions computed and both handler methods (handleChannelMenuSelect, handleShareMenuSelect) still have no test coverage — first raised in round 1, unaddressed across 3 rounds.
Gaps worth covering:
channelMenuOptionswithwindowIsSmall: truevsfalse— verifies the mobile/desktop branchinghandleChannelMenuSelect({ value: 'publish' })callspublishChannel()(not justshowPublishSidePanel = true)handleChannelMenuSelect({ value: 'delete' })callsdeleteChannelModal()handleShareMenuSelectdelegates tohandleCommonShareOption
Thanks for looking into it @vtushar06. I looked at it briefly & confirm this is most likely KDS issue. Would you like to report it in KDS repo or should I do so? |
MisRob
changed the title
|
hey @MisRob, can you please raise Issues basically what you have observed during your testing so that I can in right direction. Happy to make changes accordingly to resolve that issue. |
|
Yes I will fill a report. Thanks @vtushar06. |
rtibbles
changed the title
There was a problem hiding this comment.
Thanks @vtushar06! This is looking great!! I found one little improvement for making the options list more maintainable.
I found the cause of the dropdown option error, and opened a PR here learningequality/kolibri-design-system#1256. So this wouldn't block this PR.
Besides that, I see that on the issue description, it also mentions updating the view mode dropdown and the Add button dropdown. Did you agree to keep these places out of scope? If not, I think they should also be included in this PR. Thanks!
| } | ||
| return options; | ||
| }, | ||
| channelMenuOptions() { |
There was a problem hiding this comment.
To make this more maintainable, could you please replicate the behavior we had before using Vuetify? Instead of defining a set of options for windowIsSmall, and others when it's not, we can instead just add the actions that are hidden due to the screen size at the beginning, and then have a common list of actions. And if we can keep the same order as before, that'd be great.
There was a problem hiding this comment.
Thanks Alex, Refactored channelMenuOptions into a single common list with the toolbar-hidden actions (Publish / View details / Edit channel) prepended on small screens, matching the pre-Vuetify order. I'll keep the view-mode and Add button dropdowns out of this PR and tackle them in a separate one so this stays focused.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Mostly clean delta — the blocking error-icon fix and the loading-guard suggestion from prior reviews are resolved. One prior suggestion remains unaddressed; two new suggestions noted.
CI mostly passing (Python unit tests still pending at time of review — frontend tests, linting, and build all green).
Visual inspection: 9 screenshots cover desktop toolbar (SHARE dropdown, PUBLISH button, three-dots menu, delete modal) and mobile views. Desktop states look functionally correct. The mobile three-dots screenshots appear to be from an intermediate revision — they show "Invite collaborators" and "Share token" as separate items, but the current code's channelMenuOptions no longer includes those ShareMenuOptions values; the menu now shows "Get token" and "Share channel" instead. The current behavior matches the original pre-PR behavior (share options were always desktop-only). Updated screenshots would help confirm the mobile experience matches intent.
Prior findings
Resolved:
- Error icon lost for missing language in mobile "Edit channel details" option (blocking) —
editOption.icon = 'warningIncomplete'now set at line 516 - Missing
!loadingguard for three-dots buttonv-if(suggestion) —!loading &&prepended at line 133
Unaddressed (re-raised below):
- Missing test coverage for
channelMenuOptionsandhandleChannelMenuSelect/handleShareMenuSelect(suggestion) — no reply, no code change
2/3 prior findings resolved. 1 re-raised below.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| }, | ||
| handleChannelMenuSelect(option) { | ||
| // Check if it's a common share option first | ||
| if (Object.values(ShareMenuOptions).includes(option.value)) { |
There was a problem hiding this comment.
suggestion: This branch is unreachable — channelMenuOptions no longer adds any ShareMenuOptions values (only ChannelMenuOptions constants are pushed), so Object.values(ShareMenuOptions).includes(option.value) will always be false. The comment "Check if it's a common share option first" is now misleading. Consider removing the dead branch (and the comment) to keep handleChannelMenuSelect clean, since handleCommonShareOption is still reachable from handleShareMenuSelect.
| expect(menuOptions.some(item => item.value === 'share-token')).toBe(true); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
suggestion (re-raised — unaddressed from prior review): channelMenuOptions has significant branching logic (mobile vs. desktop items, language-warning icon, disabled publish state) and handleChannelMenuSelect routes to multiple actions — but both remain untested. The initWrapper helper already supports windowIsSmall, making it straightforward to add cases like:
channelMenuOptionsonwindowIsSmall: truevs.falsechannelMenuOptionsincludes thewarningIncompleteicon whenlanguageis missinghandleChannelMenuSelectdispatches publish / navigates to edit / opens token modal for key values
| const submitItem = menuItems.wrappers.find(item => | ||
| item.text().includes('Submit to Community Library'), | ||
| ); | ||
| const menuOptions = wrapper.vm.shareMenuOptions; |
There was a problem hiding this comment.
praise: Good call switching from DOM traversal (findAllComponents(BaseMenu), .find('.v-list__tile')) to wrapper.vm.shareMenuOptions assertions. The new tests verify computed-property outputs directly, which is more resilient to template refactors and consistent with the @testing-library philosophy the project encourages.
|
Thanks @vtushar06! Could you also confirm this, please 😅
|
| channelMenuOptions() { | ||
| const options = []; | ||
|
|
||
| // On small screens, prepend the actions that are hidden from the toolbar |
There was a problem hiding this comment.
Seems like we had an error on the before implementation, and we were not including the community library on this options list. Could you please include it inside the windowIsSmall block, please? Thanks! 🚀
|
Hi @vtushar06! Are you able to follow up on the review feedback? Also, please let us know if you’re unable to. Thank you! |
|
Hi @vtushar06! Just checking in to see if you're able to follow up on the review feedback. Please note that if we don't hear back from you, we'll assume you're unable to continue with the follow-up. Thank you! |
|
think I missed some notifications, I will procced with the follow ups thanks for letting me know @akolson. |
|
Hi @MisRob @AlexVelezLl, On the keyboard/focus issue: I went ahead and filed it in the KDS repo since it reproduces on the docs page in isolation - learningequality/kolibri-design-system#1288. Happy to pick it up there too. The remaining focus loss when a panel/modal opens looks like a separate focus-trap thing downstream of that, so I've left it out of scope here. Everything else from the last review is in now - KDS One more question: @rtibblesbot suggested adding tests for Whenever you have a moment, this should be ready for another look. |
|
Oh thanks @vtushar06! The dropdown errors were fixed in learningequality/kolibri-design-system#1256. If you rebase on top of the latest unstable the error doesn't happen anymore 🎉. (I should have flagged this in this PR, apologies 😅) Grabacion.de.pantalla.2026-06-20.a.la.s.8.21.09.a.m.mov |
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Everything else from the last review is in now - KDS warningIncomplete icon for the language warning, canShareChannel kept as a guard (CI green again), constants, useKResponsiveWindow, the !loading guard, and preview-draft on mobile. I also merged unstable for the latest KDS.
Seems like perhaps you forgot to push the new changes? 😅. The last commit on the PR is from May 14th. Besides that, did you implemented the warning icon using emojis? Or did you use the new #option slot? I think the latter would be best!
One more question: @rtibblesbot suggested adding tests for channelMenuOptions and the two select handlers, which currently aren't covered. I'm happy to add them -want me to do that in this PR, or keep it for the broader channel-editor test refactor later?
Yeah, doing it in this PR would be fine, thanks! :)
|
Thanks @AlexVelezLl! Quick clarification- the changes are already on the branch; the May 14th commit (a6e54c4) has them all (that's why CI is green), there's just no newer commit since. Sorry for the confusing wording. On the icon: it's the icon prop, not an emoji - agreed the #option slot is better, happy to switch. One scope check before I push more: the view mode and Add button dropdowns in CurrentTopicView.vue - should those be in this PR or a separate follow-up? Same BaseMenu + VList pattern, so happy to include them here, just confirming before expanding the diff. |
Yes, they should, so that we can met the acceptance criteria described on the issue 👐.
Oh, Icon should be fine, thanks! |
Summary
Migrated channel editor toolbar from Vuetify to KDS components, replacing
VTooltip,VBtn,VList, andBaseMenuwith KDS equivalents.Changes:
VTooltipwithKTooltip(error count + publish button tooltips)VBtnwithKButtonfor Publish buttonBaseMenu + VListwithKButton + KDropdownMenufor Share dropdownVBtn + BaseMenu + VListwithKIconButton + KDropdownMenufor three-dots menushareMenuOptionsandchannelMenuOptionscomputed propertieshandleShareMenuSelectandhandleChannelMenuSelectmethodsScreenshots:
…
…
…
…
…
…
…
…
…
References
Closes #5597
…
Reviewer guidance
a@a.com/a…