Skip to content
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

Implement hover tooltips in documentation #2109

Merged
merged 12 commits into from
Nov 5, 2021

Conversation

mahdienan
Copy link
Contributor

@mahdienan mahdienan commented Jul 14, 2021

This pr implements a sphinx extension to allow hovering functionality in the documentation on ReadtheDocs. The content of the text when hovering is retrieved from the glossary file which was also updated. The extension uses a sphinx role defined in _ext/HoverXTooltip.py. To make the extension match the style of NEST, we make use of

  • CSS/JS defined in bootstrap.min.css,
  • bootstrap.min.js, and
  • hoverxtooltip.css.

The name of the role is :hxt_ref:`TERM` . This role looks for descriptions of a term in the glossary.rst file and adds the first line of the definition as the content of the pop-up. The hovering function can be also used by explicitly defining the content of the pop-up by using :hxt:`TERM <hxt:CONTENT>` .

In the glossary file entries, the first line of the descriptions for the terms will be displayed as the content of the pop-up and then there must be an empty line to prevent the entire text of the description from being displayed in the pop-up.

As an example, the PyNEST tutorials have been implemented. An example build can be found here.

@jessica-mitchell jessica-mitchell added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Jul 14, 2021
@jessica-mitchell jessica-mitchell changed the title Impement Hover Tooltip for Read the Docs Implement Hover Tooltip for Read the Docs Jul 15, 2021
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mahdienan for all your hard work on this! I have a few minor English language and mark up suggestions.
Also can you please sort the glossary alphabetically (I think it's the option :sorted: but double check that).

doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
doc/userdoc/glossary.rst Show resolved Hide resolved
doc/userdoc/glossary.rst Show resolved Hide resolved
doc/userdoc/glossary.rst Show resolved Hide resolved
doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
@jougs
Copy link
Contributor

jougs commented Jul 27, 2021

@mahdienan: could you please merge master and resolve the conflict? Thanks!

@jougs
Copy link
Contributor

jougs commented Sep 5, 2021

@mahdienan: any progress on this? Thanks!

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for your contribution!

I have added some inline comments and have two more general questions:

  • Why are the three figures part of this PR? I'm fine if they have to be here, but would not want them in if they were added by accident.
  • Shouldn't this (awesome!) new feature be applied to many more files (examples, documentation files)?

doc/userdoc/_ext/HoverXTooltip.py Show resolved Hide resolved
doc/userdoc/_ext/HoverXTooltip.py Outdated Show resolved Hide resolved
doc/userdoc/glossary.rst Outdated Show resolved Hide resolved
@jessica-mitchell
Copy link
Contributor

* Shouldn't this (awesome!) new feature be applied to many more files (examples, documentation files)?

@jougs when we were discussing this project, we decided that we wanted this PR to focus mainly on the tool, with a few examples to showcase its usage. A subsequent PR will come to add this to other documentation.

@jougs
Copy link
Contributor

jougs commented Sep 16, 2021

@jessica-mitchell: fine by me. Looking forward to that PR :-)

Regarding the failing checks on GitHub Actions:

  • I already noted the missing copyright header for doc/userdoc/_ext/HoverXTooltip.py in my comments above.
  • After some investigation, it seems that we can't actually silence warnings for the failing lines in pynest/examples/clopath_synapse_spike_pairing.py due to the unwillingness of pycodestyle's developers (see # noqa ignored? PyCQA/pycodestyle#924). Please ignore for now.
  • I've just removed the whitespace from the empty line in testsuite/pytests/test_regression_issue-2069.py in master. Pulling should thus fix that (and at the same time shadow the problem above in the next run of the CI...)

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mahdienan this works well both locally and on ReadtheDocs.
I just have a couple of minor changes I'd like to see to the glossary file. Remove the subtitle 'common abbreviations in nest' and add the option :sorted: after the glossary directive

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahdienan: I still would like to see the glossary descriptions consistently in upper (my preference) or lower case. As this is only a minor point, I approve now and leave the final decision to @jessica-mitchell.

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @jougs and the descriptions that the first word should start with upper case letter.
@mahdienan can you do that pleae? I will approve also since it is a minor change.

thanks!

@terhorstd
Copy link
Contributor

Fixed last conflict that came in due to merge of #2165 . Can be merged as soon as test pass.

@terhorstd terhorstd changed the title Implement Hover Tooltip for Read the Docs Implement hover tooltips in documentation Nov 5, 2021
@jougs
Copy link
Contributor

jougs commented Nov 5, 2021

Many thanks! Merging :-)

@jougs jougs merged commit b4383bb into nest:master Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants