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

Organize the docs with cards #23746

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

asmeurer
Copy link
Member

@asmeurer asmeurer commented Jul 8, 2022

This was originally part of #23669 but I split it off due to accessibility concerns.

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

  • other
    • Use cards for the top-level sections in the documentation main page.

@sympy-bot
Copy link

sympy-bot commented Jul 8, 2022

Hi, I am the SymPy bot (v169). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • other
    • Use cards for the top-level sections in the documentation main page. (#23746 by @asmeurer)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12.

Click here to see the pull request description that was parsed.
This was originally part of https://github.com/sympy/sympy/pull/23669 but I split it off due to accessibility concerns.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- other
  - Use cards for the top-level sections in the documentation main page.
<!-- END RELEASE NOTES -->

@asmeurer asmeurer added the CZI: Documentation Issues and pull requests relating to Aaron Meurer's CZI grant documentation project label Jul 19, 2022
@bertiewooster
Copy link
Contributor

Good news: the Executable Books community will try to solve the accessibility issue.

I was trying to build the docs with cards to verify that manually applying an accessibility solution I found on StackOverflow would be spoken correctly by a screenreader, and Sphinx throws:

Extension error:
Could not import extension sphinx_design (exception: No module named 'sphinx_design')

despite sphinx-design being in doc/requirements.txt. What am I missing?

@asmeurer
Copy link
Member Author

asmeurer commented Aug 4, 2022

You need to actually install the requirements. The build doesn't do that for you automatically. The requirements file is just there to give you something to pass to pip to install everything at once.

@asmeurer
Copy link
Member Author

Looks like the accessibility concern was fixed upstream. Any other comments here? Should we try to merge this for the release, or is it too late for that?

@asmeurer asmeurer closed this Aug 18, 2022
@asmeurer asmeurer reopened this Aug 18, 2022
@asmeurer asmeurer changed the base branch from master to 1.11 August 18, 2022 21:34
@asmeurer asmeurer changed the base branch from 1.11 to master August 18, 2022 21:34
@asmeurer asmeurer closed this Aug 18, 2022
@asmeurer asmeurer reopened this Aug 18, 2022
@oscarbenjamin
Copy link
Contributor

Should we try to merge this for the release, or is it too late for that?

Can the docs not just be live updated any time?

@asmeurer
Copy link
Member Author

They could in principle. We would need to manually apply the change to the release tag, build the docs, then copy that to the site. It's not exactly a clean process compared to the current process which is all automated.

Anyway, if you don't want to make changes to the release branch this late, that's fine. This isn't a critical thing.

@oscarbenjamin
Copy link
Contributor

Should we try to merge this for the release, or is it too late for that?

Can the docs not just be live updated any time?

At release time I just update the docs by running the release/update_docs.py script locally. I use the zip file from GitHub Actions but you can also just build it locally to update the docs any other time. I guess this PR would need to be backported to the 1.11 branch before building.

@asmeurer asmeurer changed the base branch from master to 1.11 August 18, 2022 21:48
@asmeurer asmeurer changed the base branch from 1.11 to master August 18, 2022 21:48
@asmeurer
Copy link
Member Author

I was hoping we could just change the base but apparently I made this branch after the release branch, so it would have to be backported manually.

@oscarbenjamin
Copy link
Contributor

it would have to be backported manually.

It's better that way anyway. I'd rather something was reviewed and merged to master before it gets backported so that backports don't need detailed review.

@oscarbenjamin
Copy link
Contributor

Backporting a single commit is easy though:

git checkout 1.11
git checkout -b pr_backport_doc_cards
git cherry-pick doc-cards # branch name means the last commit on the branch

@github-actions
Copy link

github-actions bot commented Aug 18, 2022

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [41d90958]       [3450b757]
     <sympy-1.11.1^0>                 
-         993±3μs          600±1μs     0.60  solve.TimeSparseSystem.time_linear_eq_to_matrix(10)
-     2.85±0.02ms         1.11±0ms     0.39  solve.TimeSparseSystem.time_linear_eq_to_matrix(20)
-     5.67±0.01ms         1.62±0ms     0.29  solve.TimeSparseSystem.time_linear_eq_to_matrix(30)

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

@bertiewooster
Copy link
Contributor

bertiewooster commented Aug 18, 2022

Hmm, Lighthouse is still finding the accessibility issue Links do not have a discernible name. I noticed the PR was merged today in ExecutableBooks; it doesn't seem like they've out out a new release with it because the last release was Jun 14. Do we need to wait for a release from them?

@asmeurer
Copy link
Member Author

It won't work until they release, although if we merge this before then it will automatically start working once they do.

@bertiewooster
Copy link
Contributor

sphinx-design put out a release with this fix. @asmeurer can you rebuild the docs preview on this PR, then we can check if the cards accessibility issue is fixed?

@asmeurer
Copy link
Member Author

I restarted the Circle build, so the docs preview should be built with the latest packages.

@bertiewooster
Copy link
Contributor

Hmm, the accessibility issue Links do not have a discernible name is still there. Perhaps the latest sphinx-design release v0.3.0 didn't get pulled in?

@asmeurer
Copy link
Member Author

@asmeurer
Copy link
Member Author

I would suggest building this PR locally to confirm.

@bertiewooster
Copy link
Contributor

Unfortunately building the PR locally confirms the problem. I uploaded the built docs and still get the Links do not have a discernible name issue. I'm not sure why. sphinx-design has an example of Placing a card in a grid, it doesn't have this issue, and the rst in this PR seems to follow their format.

An example of this issue from our build is:

<a class="sd-stretched-link reference internal" href="[tutorials/index.html#tutorials](view-source:https://bertiewooster.github.io/sympy-doc/cards/tutorials/index.html#tutorials)"><span class="std std-ref"></span></a>

The link name (tutorials presumably) should be in between the <span> and </span> tags.

The above is part of the output from this rst:

.. grid:: 2

   .. grid-item-card:: Tutorials :material-sharp:`school;3em;item-card-icon`
      :link: tutorials
      :link-type: ref

      Tutorials are the best place to start for anyone new to SymPy or one of
      SymPy's features.

If we can't figure anything out, we could ask their lead developer (who coded the fix) to inspect our rst and output HTML.

@asmeurer
Copy link
Member Author

The only difference that I can see is that we are using RST whereas the example in the sphinx-design docs is presumably built from Markdown. If that's the root cause, it seems to me that would be a bug since it shouldn't matter which markup language you use as long as the underlying metadata is the same. We can try converting the index page to Markdown to see if that fixes it (see https://github.com/executablebooks/rst-to-myst).

@asmeurer
Copy link
Member Author

Just resolved a merge conflict and checked the latest build with Lighthouse, and it seems the accessibility problem is still there. Do we know what the status is of the upstream efforts to fix this?

@bertiewooster
Copy link
Contributor

bertiewooster commented Jan 21, 2023

This was fixed upstream but not in the seamless way I had hoped, which is that the alt text would automatically be set as the card title. That way, any existing cards would automatically get alt text when they upgraded sphinx-design.

The way it's implemented in sphinx-design is, there is now a link-alt option to card (and grid-item-card) directives. So it should work if you add the link-alt option to each card directive, for example

:link-alt: installation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CZI: Documentation Issues and pull requests relating to Aaron Meurer's CZI grant documentation project Documentation Merge conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants