-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Show the glossary definition when a user hovers over some Git jargon #2040
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
Conversation
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.
Wow, what a lovely feature!
Just a couple of improvements I would like to suggest:
- When hovering over a term that is scrolled too low on the page, the glossary is not shown, which is partially good because it would be displayed on top of the text. Maybe there could be a visual indicator that the glossary is hidden in such a case? Or it could be displayed on the top of the page?
- Nice touch that it works on mobile devices, too, by tapping on the term! I did notice that tapping on the same term twice keeps the glossary open; It seems to me that the convention is to hide such popups on second tap, though, maybe we should do that here, too?
That's pretty much all I could possibly contribute to make this PR even better. What an impressive feature, I really like it!
static/js/glossary.js
Outdated
}, | ||
|
||
onDataLoaded: function(data) { | ||
this.data = data['en']; // TODO: handle other languages somehow |
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.
You probably wanted to use data[this.currentLang]
here?
As to how to handle other languages, this is what I used for the multilingual search (to support searching through, say, the French pages when looking at a French manual page):
git-scm.com/assets/js/application.js
Line 247 in c239214
const language = document.querySelector("html")?.getAttribute("lang"); |
Fancy! |
f952bc7
to
46a293c
Compare
Here's an updated version with tooltips that work better, and fixing most of the other issues. You can see it in action here: https://jvns.github.io/git-scm.com/docs/git-checkout/fr I decided to truncate the glossary entry (similar to how the Wikipedia tooltips work) because I felt like it was getting unwieldy. Some things that I still need to fix:
![]() |
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.
Awesome!
@To1ne any objection against merging this in its current state? |
3019d98
to
5595da1
Compare
I think I've fixed most of the issues except for "I haven't tested it on mobile" and "maybe the term you hover over should be a link". I'm still waiting for CI to run to rebuild the man pages again. |
Updated via the `update-git-version-and-manual-pages.yml` GitHub workflow.
Updated via the `update-translated-manual-pages.yml` GitHub workflow.
198ebba
to
49bf237
Compare
After some missteps, I got the CI to work! You can see it here: https://jvns.github.io/git-scm.com/docs/git-checkout remaining issues:
|
@jvns I have no idea what we expect to happen on mobile, hover isn't a thing on touchscreens, is it? So I think that's fine (for now)
Didn't test it locally, but doesn't sound like a blocker to me. A few other comments from my side:
@dscho Overall there don't seem to be any blockers for me, so I can agree merging this version. Unless @jvns wants to address some of these comments first? |
How does one approve in GitHub?? |
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.
And people say GitLab is confusing?
I intuitively tapped on the term, and it had worked in a previous iteration.
I faced similar problems with the search, and used the
Given that it will be cached locally, this seems okay to me. Besides, it is loaded in the background, right? |
I removed the unused |
Good enough for me. |
I was thinking about how Git has a lot of jargon (
tree-ish
,pathspec
,refspec
,commit-ish
,reset
,index
, etc) which even very experienced Git users often aren't familiar with, and how Git has a glossary where (at least in theory) a lot of the jargon is defined, but that glossary isn't very discoverable. It seems like there's a design intention with the man pages where if there's some text inside angle brackets (like<tree-ish>
, then you're supposed to be able to look up that text in the glossary. What if looking it up as simple as just hovering over the term?So I hacked together a way to show the glossary entry for a piece of jargon when someone hovers over it with their mouse. You can see it in action here: https://jvns.github.io/git-scm.com/docs/git-checkout
If folks thinks this seems like an interesting idea, I can work on resolving some of the issues with the current hacked-together approach, like:
<angle brackets>
, like<tree-ish>
. It would be nice to be able to mark things outside of angle brackets as jargon, but I'm not sure how to accomplish that yetHere's a screenshot:
