feat: update readme toc button to use new component#1112
feat: update readme toc button to use new component#1112danielroe merged 7 commits intonpmx-dev:mainfrom
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! |
📝 WalkthroughWalkthroughReadmeTocDropdown.vue now uses the ButtonBase component instead of a native Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
server/utils/readme.ts (1)
456-461: Consider the payload size impact of returning raw markdown alongside HTML.Returning
markdown: contenteffectively doubles the README payload for every API response, since both the rendered HTML and the raw markdown are now transmitted. For packages with large READMEs (some exceed 100 KB), this adds non-trivial bandwidth and memory pressure on every page load — even when the user never clicks the copy button.Consider either:
- Serving the markdown via a separate on-demand endpoint (e.g.
/api/registry/readme-markdown/[...pkg]) so it is only fetched when the user clicks "Copy".- Lazy-loading the markdown field on the client side only when the copy button is clicked.
This keeps the default page load lean and only pays the cost when the feature is actually used.
app/pages/package/[[org]]/[name].vue (1)
1145-1171: Good feature implementation overall — a couple of minor observations.
aria-live="polite"on the label span (line 1161) — Nice accessibility touch; screen readers will announce the state change when the text switches to "Copied".Icon choice for the default (non-copied) state: The copy button uses
i-carbon:documentas its default icon. Other copy buttons in this file (e.g. the package-name copy button at line 550) usei-carbon:copy. Consider usingi-carbon:copyhere too for visual consistency across copy actions on the page, ori-carbon:markdownif available and you want to emphasise the markdown aspect.
ghostdevv
force-pushed
the
feat-copy-readme-to-markdown
branch
from
425c732 to
130f5f1
Compare
ghostdevv
force-pushed
the
feat-copy-readme-to-markdown
branch
from
130f5f1 to
d3ecc15
Compare
ghostdevv
force-pushed
the
feat-copy-readme-to-markdown
branch
from
d3ecc15 to
4c9238e
Compare
ghostdevv
changed the title
ghostdevv
force-pushed
the
feat-copy-readme-to-markdown
branch
from
4c9238e to
6f50f2d
Compare
|
|
Deployment failed with the following error: |
|
Deployment failed with the following error: |
|
Deployment failed with the following error: |
ghostdevv
force-pushed
the
feat-copy-readme-to-markdown
branch
from
a48379f to
a110b4a
Compare
ghostdevv
changed the title
|
This poor PR has been through a lot, but I'm determined to save it! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/components/ReadmeTocDropdown.vue (2)
83-88:⚠️ Potential issue | 🔴 CriticalFix
buttonRefaccess — requires.valueto dereference the exposed ref.
buttonRefexposed fromButtonBaseis the rawRef<HTMLButtonElement>returned byuseTemplateRef. Vue does not auto-unwrap refs accessed on component instances viadefineExpose. Therefore,triggerRef.value.buttonRefreturns the Ref object itself, not the HTMLButtonElement.Lines 84, 105, and 142 currently call
.getBoundingClientRect()and.focus()directly on the Ref object, which lacks these DOM methods and will throw a TypeError at runtime.The guard on line 83 is also incorrect—it checks if the Ref object exists (always truthy), not if the element exists.
All three usages require
.valueappended:Proposed fix
function toggle() { if (isOpen.value) { close() } else { - if (triggerRef.value?.buttonRef) { - const rect = triggerRef.value?.buttonRef.getBoundingClientRect() + const buttonEl = triggerRef.value?.buttonRef?.value + if (buttonEl) { + const rect = buttonEl.getBoundingClientRect() dropdownPosition.value = { top: rect.bottom + 4, right: rect.right, } }And fix lines 105 and 142:
- triggerRef.value?.buttonRef?.focus() + triggerRef.value?.buttonRef?.value?.focus()
115-115:⚠️ Potential issue | 🟠 Major
onClickOutsideignore requires the actual DOM button element, not the component instance ref.
triggerRefis a component instance ref toButtonBase. VueUse'sonClickOutsideignoreoption expects DOM element refs (like those fromuseTemplateRef()) or selector strings, not component refs. You should either useunrefElement(triggerRef)with a computed, pass the exposedbuttonRefdirectly, or use a selector string.For example:
-onClickOutside(listRef, close, { ignore: [triggerRef] }) +onClickOutside(listRef, close, { ignore: [() => unrefElement(triggerRef)] })Or, since
ButtonBaseexposesbuttonRef:-onClickOutside(listRef, close, { ignore: [triggerRef] }) +onClickOutside(listRef, close, { ignore: computed(() => [triggerRef.value?.buttonRef?.value]) })
ghostdevv
force-pushed
the
feat-copy-readme-to-markdown
branch
from
d9b4325 to
7152c10
Compare
|
@danielroe I totally just overwrote your commit 🤦♀️ force-with-lease isn't working and regardless I forget we squash merge!! I hopefully have done the same result? |
app/components/Button/Base.vue
Outdated
|
|
||
| defineExpose({ | ||
| focus: () => el.value?.focus(), | ||
| buttonRef: el, |
There was a problem hiding this comment.
previously only the focus was exposed to avoid too much dependency externally on the inner workings of the button, so I wonder if it would be better simply to expose getBoundingClientRect and/or consider whether we can refactor in future.
|
you did exactly what I did 👌 |
Co-authored-by: Willow (GHOST) <git@willow.sh> Co-authored-by: Daniel Roe <daniel@roe.dev>
3 participants
Update the README toc button to use the new button components
(original description before pr change)
Add a button to copy raw readme markdown to the clipboard
screen recording
copy_readme_markdown_compressed.mp4
closes #1020there is already a PR (#1058) addressing this iddue, I found that out just before creating this PR, I am opening this PR as there are some subtle design differences, not trying to compete or anything, just for a design consideration, I am okay even if my design is selected but implemented in the first PR.