-
Notifications
You must be signed in to change notification settings - Fork 191
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
Docs: scaffolding for the "Intro" section #4011
Docs: scaffolding for the "Intro" section #4011
Conversation
Co-authored-by: Leopold Talirz <leopold.talirz@gmail.com>
Codecov Report
@@ Coverage Diff @@
## docs-revamp #4011 +/- ##
===============================================
- Coverage 78.46% 78.45% -0.01%
===============================================
Files 461 461
Lines 34077 34077
===============================================
- Hits 26736 26732 -4
- Misses 7341 7345 +4
Continue to review full report at Codecov.
|
Regarding 6.:
I don't think we can get rid of the details directive dependency completely, as it's also used in the |
One thing that I wanted to briefly mention, although it is most probably not something I expect us to implement, is that currently using CircleCI for documentation integration testing has a big benefit over GitHub Actions, because it can provide HTML build artefacts. For example in: executablebooks/sphinx-panels#3, you get a link that directly opens the newly built documentation for review: |
That's a really nice feature and we could consider to use circle-ci specifically for this purpose. @chrisjsewell I'm wondering whether it would be possible to implement something similar in GitHub actions as well though. Uploading artifacts is not that difficult, I just don't know about "serving" the HTML so that you can just open it directly without downloading first. |
Its just better looking TBH, and mirrors better the pandas doc section that we were looking to replicate. Note also that the code/css follows closely this guide: https://www.w3schools.com/howto/howto_js_collapsible.asp |
Unfortunately at the moment no. I would refer you to this issue, roughly tracking the status of this feature in Github actions: spatialaudio/nbsphinx#361 |
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.
Really nice! First set of recommendations to change up the language a little bit on the landing page. I'll start reviewing the installation instructions next.
Yeah I'm not against this in principle, just wondering if the same (in terms of style) could be achieved with custom CSS for the details tag. Plain HTML has the advantage of also working for the subset of users who disable JS. |
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.
Provided a few suggestions for the conda installation instructions.
docs/source/intro/get_started.rst
Outdated
Getting Started | ||
**************** | ||
|
||
Installation |
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.
Installation | |
Install with conda |
And that should then be on its own page.
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 not clear what you are expecting to remain on this page. If we move the conda install to a separate page, then surely there will be nothing left on this page?
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.
The panels for the main installation routes is left.
docs/source/intro/get_started.rst
Outdated
There are numerous routes to setting up the full AiiDA environment, depending on your use case. | ||
Here we first provide the simplest approach for installation on your local computer. |
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 think this paragraph should remain on the "Getting started" landing page after we have moved the conda instructions to a separate page.
docs/source/intro/get_started.rst
Outdated
|
||
$ conda create -n aiida -c conda-forge aiida-core aiida-core.services | ||
$ conda activate aiida | ||
$ reentry scan |
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.
Could this be rolled into conda activate aiida
?
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.
Possibly. Me and @ltalirz have already discussed this for example in chrisjsewell/activate_aiida#4, and I think he is looking into it
docs/source/intro/get_started.rst
Outdated
.. code-block:: console | ||
|
||
$ conda create -n aiida -c conda-forge aiida-core aiida-core.services | ||
$ conda activate aiida |
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 activates the services, correct? I think we should either move this into a separate code block or mention right after, that this will need to be run after every reboot. Ideally, we can avoid this all-together in the future, but for now we need to mention that.
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 activates the services, correct?
No this is just activating the conda enviroment (aiida is the environment name, maybe that is misleading?). It does need to be run when "entering" a new terminal though, i.e. to set the environment
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.
Sorry, I completely misread this, sorry.
docs/source/intro/get_started.rst
Outdated
.. note:: | ||
|
||
You can also install ``aiida-core`` from `PyPi <https://pypi.org/project/aiida-core>`_ |
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 think we should be more generic here and just say "Click here for alternative installation methods.". This is for users who were directed to this page (e.g. from a Stack Overflow answer or some other source), but actually don't want to use conda.
.. code-block:: console | ||
|
||
$ initdb -D mylocal_db | ||
$ pg_ctl -D mylocal_db -l logfile start |
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.
Could this be rolled into conda activate aiida
?
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.
see above comment
|
||
.. code-block:: console | ||
|
||
$ rabbitmq-server -detached |
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.
Similar question, make part of conda activate aiida
?
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.
see above comment
@greschd I agree that this should work without JS, but I would be in favor of creating an issue for that and then resolving that down the line. Our priority right now should be to get something online as soon as possible. |
Err, well not according to this blog: Why <details> is Not an Accordion. I guess that's a group decision on whether to support JS, to improve the style. I'm in favour, and it appears pandas are as well 😬 On a related note (again really for a separate issue) pandas does not require the custom JS, because it uses a "more modern" bootstrap theme (see here re collapse), which I am in favour of moving to: https://pypi.org/project/pydata-sphinx-theme/ |
Ok, if I understand correctly the problem is that headings inside To be honest I'm also a bit out of my depth here, frontend is not really my specialty. |
Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>
Co-authored-by: Carl Simon Adorf <carl.simon.adorf@gmail.com>
…sewell/aiida_core into docs-revamp-intro-install
|
||
.. code-block:: console | ||
|
||
$ reentry scan |
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 have figured out how best to succinctly explain this command yet
The intro section contains the landing page with information about motivation and scope of the AiiDA software, as well as an overview of the documentation structure with links to other sections. This intro section also contains the instructions for how to install and setup AiiDA.
The intro section contains the landing page with information about motivation and scope of the AiiDA software, as well as an overview of the documentation structure with links to other sections. This intro section also contains the instructions for how to install and setup AiiDA.
Aim
From wiki:
Outline
From Google Doc Outline, it should follow this outline:
index
): Like pandas (https://pandas.pydata.org/docs/index.html)intro/get_started
): final state: verdi status is green (+ localhost computer is setup?)intro/installation
): incorporate all existing documentation +intro/troubleshooting
): incorporate existing troubleshootingOutstanding Issues
intro/about
section. The features section of the landing page has currently been deleted, but this should be added back in, in some form. Also relevant is that @ltalirz is working on a testimonial section.sphinx-panels
dependencysphinxcontrib-details-directive
dependency, and should get consensus that this is OK.Note some initial discussion has already taken place in csadorf#25