feat: copy compare table as markdown#1533
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
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:
📝 WalkthroughWalkthroughAdds a new Vue component CopyToClipboardButton (script setup, TypeScript) and replaces several ad-hoc floating copy buttons with it across the site. Adds exportComparisonDataAsMarkdown() to build a Markdown table from the comparison grid and copy it to the clipboard, and wraps the comparison header with CopyToClipboardButton. Introduces i18n keys/schema entries for copy_as_markdown and updates lunaria locale files. Adds accessibility tests for CopyToClipboardButton and removes the previous floating copy-button styles/markup on the package page. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server/utils/docs/text.ts (1)
121-124: Avoid the non‑null assertion on array indexing.The guideline calls for explicit checks when indexing arrays; this avoids
!and keeps the code strictly type‑safe.♻️ Suggested fix
- for (let i = 0; i < codeBlockData.length; i++) { - const { lang, code } = codeBlockData[i]! + for (let i = 0; i < codeBlockData.length; i++) { + const entry = codeBlockData[i] + if (!entry) continue + const { lang, code } = entry const highlighted = await highlightCodeBlock(code, lang) result = result.replace(`__CODE_BLOCK_${i}__`, highlighted) }As per coding guidelines, "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
app/pages/compare.vue (1)
95-100: Consider adding a type guard for safer array access.Accessing
children[0]at line 97 could returnundefinedif the grid has no children. While line 100 handles the falsy case, TypeScript may not narrow the type correctly. The current logic works but could be made more explicit.Proposed defensive check
function gridToMarkdown(gridEl: HTMLElement): string { const children = Array.from(gridEl.children) - const headerRow = children[0] + const headerRow = children[0] as Element | undefined const dataRows = children.slice(1) if (!headerRow || dataRows.length === 0) return ''app/components/CopyToClipboardButton.vue (1)
51-83: Consider adding graceful degradation for older browsers withoutallow-discretesupport.The
allow-discretetransitions are well-supported across modern browsers (Chrome 117+, Safari 17.4+, Firefox 129+), but this is a feature projected to reach "Baseline widely available" by February 2027. For enhanced compatibility with older browser versions, consider either removingallow-discretefrom production builds or implementing a progressive enhancement strategy. Without it, transitions become discrete jumps rather than smooth animations—the component remains functional, but the visual refinement is lost.
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
although these were already dependencies, I don't think any of them were used in the client.
I think there must be a better way to render the table to markdown without converting from html - as we have the source for that html, the most complex piece here would only be padding to ensure the columns are the same width
I see no issue with changing this and can update the code to just generate markdown from the client side data without doing the entire My idea was here to create a function we could re-use in the future while allowing this functionality as the first one using it. |
|
thank you ❤️
let's wait until we see if we need it in other cases - we can refer back to this PR for implementation if so. 🙏 |
|
100%, will update once I have a moment :) |
…with imports, reverted packages, new table to md function
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lunaria/files/en-US.json (1)
915-915: Optional: consider aligning the label with the existingcopy_as_markdownpattern.The sibling key
package.readme.copy_as_markdown(line 249) uses the value"Copy README as Markdown", which tells the user about the output format. The newcompare.packages.copy_as_markdownvalue"Copy table"omits any mention of Markdown, so users pasting into a plain-text editor may be surprised by the raw Markdown syntax.
"Copy table as Markdown"(or simply"Copy as Markdown") would be consistent with the existing pattern and make the output format explicit. This should be addressed in the basei18n/locales/en.jsonas well, sincelunaria/files/en-US.jsonmirrors it.
|
@danielroe updated Since you mentioned padding, do you think the link in the package headers should be removed? I padded only to names.
|
|
@gameroman hi, since you proposed this, what's your stance on URLs in headers package names? |
I think without them |
|
@gameroman updated :)
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/pages/compare.vue (1)
93-112:indexparameter in the innerforEachshadows the outer facetindex.Both the outer
selectedFacets.value.forEach((facet, index)and the innermdData?.[index + 1]?.forEach((item, index)useindex. While the outerindexis evaluated correctly before the inner loop begins (inmdData?.[index + 1]), the shadowing makes the code harder to reason about and could silently break if the inner reference is ever moved inside the callback.✏️ Suggested rename
- mdData?.[index + 1]?.forEach((item, index) => { - if (item.length > (maxLengths?.[index] || 0)) { - maxLengths[index] = item.length + mdData?.[index + 1]?.forEach((item, colIndex) => { + if (item.length > (maxLengths?.[colIndex] || 0)) { + maxLengths[colIndex] = item.length } })
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/pages/compare.vue (2)
244-263:⚠️ Potential issue | 🟠 Major
<h2 id="comparison-heading">hidden on mobile breaks the section'saria-labelledbylabel.When data is present, the heading is inside the
CopyToClipboardButtonwhich carrieshidden md:inline-flex. Below themdbreakpoint the element (and itsid) is removed from the accessibility tree, leaving<section aria-labelledby="comparison-heading">with no accessible name on mobile. Thev-elsefallback heading is only rendered when there is no data, so there is no mobile fallback when data is loaded.
248-249:⚠️ Potential issue | 🟡 Minor
inline-blockis never applied — dead utility class.
hiddensetsdisplay: noneat the default breakpoint, overridinginline-blockbefore it can take effect. Atmd+,md:inline-flextakes over.inline-blockis unreachable.✏️ Proposed fix
- class="mb-4 inline-block hidden md:inline-flex" + class="mb-4 hidden md:inline-flex"
🧹 Nitpick comments (1)
app/pages/compare.vue (1)
107-111:indexin the innerforEachshadows the outerforEach'sindex.The inner callback reuses
indexas the cell index, which shadows the outerindex(the facet index). It works correctly here because the outerindexis only needed formdData?.[index + 1]before the inner callback body, but it is a maintenance hazard.♻️ Proposed rename
- mdData?.[index + 1]?.forEach((item, index) => { - if (item.length > (maxLengths?.[index] || 0)) { - maxLengths[index] = item.length + mdData?.[index + 1]?.forEach((item, cellIndex) => { + if (item.length > (maxLengths?.[cellIndex] || 0)) { + maxLengths[cellIndex] = item.length } })
|
Looks very good so far |
3 participants
Implements #1515
Packages used for html to markdown util are dependencies that already were used in the project.
How it work:
Screencast_20260216_192439.mp4