fix: apply middle-ellipsis modifier classes to prevent double-truncation - #800
Open
16navigabraham wants to merge 1 commit into
Open
Conversation
Closes CalloraOrg#750 Note on scope: the issue references src/pages/PlanSelector.tsx, which does not exist anywhere in this repo (confirmed via full-text and git history search). The middle-ellipsis breadcrumb feature the issue describes was already fully implemented in src/components/Breadcrumb.tsx by a prior contribution (issue CalloraOrg#567, demonstrated via src/pages/RateLimitCard.tsx) — truncateMiddle(), the collapsed-items popover, keyboard nav, and full title/aria-label preservation were all already in place and already covered by 27 existing tests. What was actually missing: the stylesheet defines .breadcrumb-link--middle-ellipsis / .breadcrumb-current--middle-ellipsis modifier classes specifically to disable the CSS-level text-overflow: ellipsis once JS has already shortened a label — the comment directly above them explains this prevents a "start…en…" double-truncation artefact when the JS-truncated string still doesn't fit an extremely narrow container. But the JSX never actually applied either modifier class, so that protection was dead code. This also applied to the popover's expanded-item links, which had no modifier class defined for them at all. Fixed by: - Applying breadcrumb-link--middle-ellipsis / breadcrumb-current--middle-ellipsis to BreadcrumbLink when the label is actually truncated. - Adding a matching breadcrumb-popover-link--middle-ellipsis modifier (CSS + JSX) for the popover's expanded items, which had the identical gap. Tests: 5 new cases in Breadcrumb.test.tsx confirming the modifier class is present exactly when a label is truncated (first-crumb link, current-page crumb, popover links) and absent when it isn't. npx vitest run src/components/Breadcrumb.test.tsx Test Files 1 passed (1) Tests 34 passed (34)
|
@16navigabraham Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
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.
Closes #750
Note on scope
The issue references
src/pages/PlanSelector.tsx, which does not exist anywhere in this repo (confirmed via full-text search and git history). The middle-ellipsis breadcrumb feature the issue describes was already fully implemented insrc/components/Breadcrumb.tsxby a prior contribution (issue #567, demonstrated viasrc/pages/RateLimitCard.tsx) —truncateMiddle(), the collapsed-items popover, keyboard navigation, and fulltitle/aria-labelpreservation were all already in place and already covered by 27 existing tests.What was actually missing
The stylesheet defines
.breadcrumb-link--middle-ellipsis/.breadcrumb-current--middle-ellipsismodifier classes specifically to disable the CSS-leveltext-overflow: ellipsisonce JS has already shortened a label — the comment directly above them explains this prevents a"start…en…"double-truncation artefact when the JS-truncated string still doesn't fit an extremely narrow container. But the JSX never actually applied either modifier class, so that protection was dead code. The same gap existed for the popover's expanded-item links, which had no modifier class defined for them at all.Fix
breadcrumb-link--middle-ellipsis/breadcrumb-current--middle-ellipsistoBreadcrumbLinkwhen the label is actually truncated (not unconditionally).breadcrumb-popover-link--middle-ellipsismodifier (CSS + JSX) for the popover's expanded items, which had the identical gap.Tests
5 new cases in
Breadcrumb.test.tsxconfirming the modifier class is present exactly when a label is truncated (first-crumb link, current-page crumb, popover links) and absent when it isn't.Repo-wide
npx vitest runhas 68 pre-existing failures across 13 files (confirmed unrelated — none touchBreadcrumb.tsx/Breadcrumb.test.tsx).