fix(ui): keep compare header links inside their grid column#2805
fix(ui): keep compare header links inside their grid column#2805rohitg00 wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdd ChangesHeader link flex layout and overflow constraint
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hello! Thank you for opening your first PR to npmx, @rohitg00! 🚀 Here’s what will happen next:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
graphieros
left a comment
There was a problem hiding this comment.
@rohitg00 Thank you for your PR :)
From what I can see, long package names still bleed, and the start of the name can also appear cropped in some cases:
I think using ellipsis for package names in the headers can be a good alternative (it is already used in other places in the compare page, inside the charts for example).
Replace break-words + line-clamp-1 with truncate (single-line ellipsis) on the package name and version spans, matching the pattern already used for chart labels in the same page. Previously, scoped names like @agentmemory/agentmemory broke at the '@' boundary so the scope prefix wrapped onto a second line that line-clamp-1 hid, making it look like the start of the name was cropped. Truncation keeps the start visible and ends in '…' when the name overflows.
|
Thanks @graphieros — you're right, the previous fix didn't help with long names. Pushed 8f56990 to switch the name + version spans to Root cause of the cropped start in your screenshot: |
| it('constrains the header link to its grid track so long scoped names cannot overflow', async () => { | ||
| const scopedName = '@scope/very-long-package-name' | ||
| const scopedVersion = '1.2.3' | ||
| const component = await mountSuspended(ComparisonGrid, { | ||
| props: { | ||
| columns: [ | ||
| { name: scopedName, version: scopedVersion }, | ||
| ...cols('pkg-b@1.0.0', 'pkg-c@1.0.0', 'pkg-d@1.0.0'), | ||
| ], | ||
| }, | ||
| }) | ||
|
|
||
| const link = component.find(`a[title="${scopedName}@${scopedVersion}"]`) | ||
| expect(link.exists()).toBe(true) | ||
| expect(link.classes()).toContain('flex-1') | ||
| expect(link.classes()).toContain('min-w-0') | ||
| }) |
There was a problem hiding this comment.
not sure this test makes sense
There was a problem hiding this comment.
I agree, the css change is enough and does not require coverage in my opinion.
|
I suspect your previous answer (if not this PR) was generated by an agent. Therefore I would be happy to merge your PR (probably without the test, if you were so kind to consider its removal), should you answer to this message in your own words. |
|
Hey! @graphieros Yeah, I use Agents a lot. I made this PR with Claude code, but I guided it and faced problem myself when I was comparing different memories with my agentmemory project. Thanks, happy to help. |
|
Thanks, and nice to meet you :) |
graphieros
left a comment
There was a problem hiding this comment.
(see remarks regarding the test)
Per reviewer feedback: the CSS change covers the behavior; asserting on flex-1/min-w-0 class names doesn't add meaningful coverage.
|
Agreed @gameroman @graphieros : dropped that test in f29eb4b. Kept only the title-attribute / render assertion. |
graphieros
left a comment
There was a problem hiding this comment.
LGTM
Thank you for dropping the em dash in that last comment ;)
I hate emdashes. |
Fixes #2802
On the compare page, package headers like
@agentmemory/agentmemoryran wider than their grid column and visibly overlapped both the sticky label cell on the left and the next package header on the right.The header link in
ComparisonGrid.vuewasflex min-w-0 flex-colwith noflex-1/w-full, so it sized to its intrinsic content width and escaped the grid track. Addingflex-1makes the link fill the available track width, which gives the innerline-clamp-1spans a real width to clamp against. The existingmin-w-0already lets it shrink when the column is narrower than the name.Repro URL: https://npmx.dev/compare?packages=@agentmemory/agentmemory,claude-mem,supermemory,mem0ai&layout=split
I also added a small regression test that mounts the grid with the four packages from the screenshot above and asserts the long-name header link carries
flex-1andmin-w-0.