-
Notifications
You must be signed in to change notification settings - Fork 48
Copy section URL on section hover chain icon click #4386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Copy section URL on section hover chain icon click #4386
Conversation
✅ Deploy Preview for viam-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…fusing changelog partial
@JessamyT added the copy behavior to changelog and glossary! I'm seeing some strange glossary behavior on my local machine -- seems like not all of the summaries load. So keep an eye out for that on the staging preview in case that's not a local phenomenon. FYI, it looks like the Hugo Scratch method has been deprecated. Aliased to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen the summaries not loading in local build before (with prod being fine) so that's probably okay?
The links seem to work so LGTM
assets/js/base.js
Outdated
checkmark.classList.add('visible'); | ||
|
||
setTimeout(() => { | ||
checkmark.classList.remove('visible'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it say copied? A bit like the code samples do? That would be more using existing patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change to be closer to copy behaviour. I think displaying a checked mark does not really tell me "copied"
@npentrel switched to 'copied' text, slightly smaller than the title, styled similarly to our current code samples & italicized to obviously differentiate it from the section headers. Currently displays for 2 seconds after the user clicks the permalink. Taking advantage of built-in Docsy hooks to re-implement the (client-side!!) javascript that currently generates these self-links, instead of applying the onclick method to the entire heading. Let me know if you'd like the heading to be clickable as well -- I stuck with this because this approach effectively behaves the exact same way as the production site, just with additional copying to clipboard and the little copy notification. Let me know if you'd like the styling to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better and I'll approve so that I'm not blocking merge. However the styling is a bit off. It is centered compared to the anchor marker but it would make more sense to center it based on the header. It would also be good if the anchor showed for the same 2 seconds, it currently disappears and then copied is still visible which feels a bit odd to me
Good point, I'll tweak the alignment so it's centered based on the header. I went with this because I was getting some strange alignment relative to monospace headers previously, but if I can figure that out I think it would look better, too. |
@npentrel tweaked the alignment and forced the link icon to display while the copied notification shows, let me know what you think! |
I've heard from a few people that they expect the chain icon that appears when you hover over a section header (h1, h2, h3, etc) to copy the anchor link when you click it. It currently navigates to that anchor point and updates your URL in the URL bar of your browser. This adds the copy functionality.