-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix broken math link icon #1191
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
|
|
||
| // This is the #href that shows up on hover. Sphinx's is terrible so I hack it away. | ||
| h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption | ||
| h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption, .eqno |
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 CSS selector seems a bit too specific in the future we may want to use just .headerlink
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'm guessing this list is just for applying the opacity on the parent element, otherwise we might be able to use .headerlink, yeah. This seems fine for now.
We could also use the entire .math parent element instead here, so that hovering over the math element will give the anchor link. I'm not sure how folks normally use the equation number here, but it might be hard to discover this anchor link if the hover event is just on the equation number.
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.
Looks better! It is a bit hard to find the link to the equation now, though not sure how useful the link was before. I could see however on the parent .math class working well too.
|
|
||
| // This is the #href that shows up on hover. Sphinx's is terrible so I hack it away. | ||
| h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption | ||
| h1, h2, h3, h4, h5, h6, dl dt, p.caption, table > caption, .code-block-caption, .eqno |
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'm guessing this list is just for applying the opacity on the parent element, otherwise we might be able to use .headerlink, yeah. This seems fine for now.
We could also use the entire .math parent element instead here, so that hovering over the math element will give the anchor link. I'm not sure how folks normally use the equation number here, but it might be hard to discover this anchor link if the hover event is just on the equation number.
I agree this is something I noticed but wasn't sure if we want to go out of the scope of the bugfix here. Making it appear when covering the whole math equation is also a solution though. Maybe this should be tackled in 1.1 as part of: #383 |
|
This looks good! Also :bump to # 383 as part of 1.1. |
|
Sounds good, let's address it more in 1.1 |
This likely was indirectly broken with da86e4d
This commit addes our standard headerlink icon styles to math links aswell, before they had no styling.
Fixes #1189