Refined prev/next buttons in course viewer#14399
Refined prev/next buttons in course viewer#14399nucleogenesis wants to merge 2 commits intolearningequality:release-v0.19.xfrom
Conversation
github-actions
bot
added
APP: Learn
| class="btn-flex" | ||
| appearance="flat-button" | ||
| data-testid="prev-button" | ||
| :style="{ gap: showButtonLabels ? '8px' : '0px' }" |
There was a problem hiding this comment.
This was a bit annoying but it's due to KButton internal <span>s being present even if empty, so the gaps would be rendered and the arrow off-center unless I cleared the gap value like this.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean styling refinement that matches the Figma spec. Screenshots look good at both wide and narrow viewports — icons are aligned with text, spacing and font weight match the design.
CI still in progress. PR targets release-v0.19.x; no new strings added.
- suggestion:
flex-grow: 1on.btn-icon(line 185) — see inline comment - praise: Good use of
#icon/#iconAfterslots and theme tokens
@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
|
|
||
| .btn-icon { | ||
| top: -2px; | ||
| flex-grow: 1; |
There was a problem hiding this comment.
suggestion: flex-grow: 1 on the icon element is surprising — it causes the icon's box to expand to fill remaining space in the flex container, though the SVG stays 16×16. If the intent is just to size and position the icon, flex-shrink: 0 (prevent shrinking) is more conventional. If this is compensating for a specific KButton slot layout quirk, a brief comment explaining why would help future readers.
| width: 16px; | ||
| height: 16px; | ||
|
|
||
| &.hotfixed { |
There was a problem hiding this comment.
praise: Good practice documenting the hotfix with a link to the upstream KDS PR — makes it easy to find and clean up later.
| class="icon" | ||
| icon="back" | ||
| class="btn-icon" | ||
| :color="$themeTokens.text" |
There was a problem hiding this comment.
praise: Nice use of $themeTokens.text and $themeTokens.textInverted for the icon colors — keeps theming consistent.
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean delta — prior flex-grow suggestion resolved in 2b923d7. CI passing (core checks green; APK/DMG/Pi builds still pending but unrelated to this change). Screenshots confirm proper alignment and spacing at both wide and narrow viewports.
Prior review findings:
flex-grow: 1on.btn-icon— RESOLVED (removed in latest commit)
No new findings. LGTM.
@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
| class="btn-flex" | ||
| appearance="flat-button" | ||
| data-testid="prev-button" | ||
| :style="{ gap: showButtonLabels ? '8px' : '0px' }" |
There was a problem hiding this comment.
praise: Pragmatic workaround for the KButton empty-span gap issue — the inline gap toggle keeps the icon centered without needing a wrapper element.
2 participants
Summary
Applies styles to match those in the Figma for PrevNextBar.vue and aligns arrow icons w/ their text. Some specific changes I gleaned from Figma and other notes:
iconAfterslot in samespan.icon-containeroniconslot kolibri-design-system#1219)References
Closes #14220
Reviewer guidance
Check out a course as a learner and see the sweet new button styles!