KToolbar cleanup [Companion to KDS#1236]#14618
Draft
nucleogenesis wants to merge 5 commits into
Draft
Conversation
- AppBar: drop removed props (removeNavIcon, type, removeBrandDivider) and add backgroundColor: 'transparent' to :style to preserve the prior type="clear" behavior (the wrapper div still supplies the themed background) - ImmersiveToolbar: drop removed props (textColor, type, showIcon, @nav-icon-click). Fold textColor into :style using $themeTokens.text / $themeTokens.textInverted to match the tokens already used on the nested KIconButton :color bindings. Also drops the internal showIcon prop declaration — no caller ever set it and the only usage was on the removed KToolbar :showIcon binding - ContentModal: drop showIcon (never was a real KToolbar prop) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Temporary dependency pin so this PR's CI builds against the KDS cleanup PR before it's released. Must be reverted to a proper version bump (e.g., 6.0.0) once KDS merges and publishes. Points the pnpm catalog entry to github:nucleogenesis/kolibri-design-system-1 on branch 1183--ktoolbar-cleanup. Regenerated pnpm-lock.yaml accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
APP: Learn
Contributor
npm Package VersionsMerging this PR will publish the following packages to npm:
Warning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
Contributor
Build Artifacts
Smoke test screenshot |
AlexVelezLl
reviewed
Apr 23, 2026
Member
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
looks nice, just a comment as a reminder for ourselves that we also need to update the KTooblar usage on ResourceLayout, and we should also be able to remove this deep selector, and these deep selectors on the appbar
…t + CSS class rename) KDS PR learningequality#1236 picked up additional breaking changes during review that this branch's earlier migration commit didn't anticipate. Mirror them on the Kolibri side: - #icon slot → #leading-actions, #actions slot → #trailing-actions on direct KToolbar children. Updated in AppBar, ContentModal, ImmersiveToolbar (also keeping its own outward-facing #actions slot name unchanged so its consumers don't have to migrate too), and LearningActivityBar (only the two templates that are direct KToolbar children — KLabeledIcon's nested #icon slots are untouched). - /deep/ .k-toolbar-nav-icon → /deep/ .k-toolbar-leading-actions in the matching SCSS rules (ImmersiveToolbar, LearningActivityBar). - Drop ImmersiveToolbar's /deep/ .k-toolbar-title { text-overflow: ellipsis; } rule — KToolbar now bakes in title truncation via KTextTruncator, so the workaround is redundant. Out of scope here, tracked separately: - The wholesale /deep/ removal across the repo (190 usages); the remaining /deep/ .k-toolbar-* rules in these files stay for now. - AppBar.vue's sub-nav migration to KToolbar's new #extension slot (needs a redesign of the dual-render Navbar overflow logic).
5 tasks
Drops the wrapper div that was supplying the toolbar's themed background (backgroundColor moves directly onto KToolbar's :style), moves the sub-nav Navbar from a sibling 'subpage-nav' div into KToolbar's new #extension slot, and removes the duplicate inline Navbar that was living in the #navigation slot for overflow measurement. Also drops the dual-render plumbing that supported the now-defunct measurement copy: overflowCount data, showTopNavBar computed, and hiddenNavbarStyle computed. The single Navbar instance in #extension gets full container width and computes its own overflow without needing a measurement twin.
Lockfile was pinned to afa62dac4 — a SHA that no longer exists on the KDS branch after force-pushes. Updates to ba795be36 (current tip). Tests that were failing because the locally-installed KDS still had the old #icon/#actions slot names now pass. Will be reverted to a published version bump once KDS PR learningequality#1236 lands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Companion PR to kolibri-design-system#1236. Updates Kolibri's three
KToolbarconsumers (AppBar,ImmersiveToolbar,ContentModal) to drop props removed in the KDS cleanup, and adds an explicitbackgroundColor: 'transparent'onAppBarto preserve the transparency that the removedtype="clear"used to provide.References
Reviewer guidance
Intentionally zero-behavior-change against the currently-installed KDS version — the companion is safe to land before KDS #1236 ships. Once KDS ships with the removed props, Kolibri will also need a
kolibri-design-systemversion bump (separate PR).Key things to look at:
AppBar.vue:14-21—backgroundColor: 'transparent'is intentionally added to:style. KDS Coaches should not be required to be members of the classes for which they are the coach #1236's newKToolbarsupplies a$themeTokens.surfacebackground by default; without this override, the AppBar would render a white toolbar instead of letting the wrapper div'sthemeConfig.appBar.backgroundshow through. This preserves the effect of the removedtype="clear".ImmersiveToolbar.vue— the prior:textColor="isFullscreen ? 'black' : 'white'"is nowcolor: isFullscreen ? $themeTokens.text : $themeTokens.textInvertedinside:style. Matches the theme tokens already used by the nestedKIconButton :colorbindings. Fine on default theme; worth a quick visual check if your fork uses a non-default palette.ImmersiveToolbar.vueprop removal — the internalshowIconprop is deleted. No caller in this repo ever set it, and its only consumption was the removed:showIconKToolbar binding.@nav-icon-clickwas removed from the<KToolbar>binding. ThenavIconClickemit still fires from the nestedKIconButton @clickhandlers, soImmersivePage's@navIconClicklistener still receives events.Note on the
[TEMP]commit: pins thekolibri-design-systempnpm catalog entry to the KDS PR branch so CI actually builds against the cleanup. Will be reverted to a proper version bump once KDS merges and publishes.Suggested test paths in a dev run:
AppBarrenders with themed backgroundImmersiveToolbar(bothisFullscreen=trueandfalsepaths)ContentModalLearningActivityBaris untouchedAI usage
Used Claude Code to audit downstream
KToolbarusage across Kolibri, draft the template edits, catch the AppBar transparency regression during review, and compose this PR body. Verified changes locally.