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

👌 Improve cross-theme compatibility of need collapse button #1181

Merged
merged 3 commits into from
May 15, 2024

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented May 15, 2024

Currently, the need collapse toggle icons are added to the page like:

image

Wrapping the SVG icons in an <img src=...> is not great, because it does not allow it to be properly styled.
Indeed for our own docs, we have to use the horrible hack of filter: brightness(0) invert(1), to get these icons to have the correct color for light/dark mode 😬

This PR instead places the entire SVG onto the page, and adds some core CSS to set it at the correct relative size (and float right)

image

the SVG then matches the local text color, without any need for additional CSS:

image

Also, the initial visibility of the icons is now set correctly, before any JS is run,
so that you don't get a bad render experience, whereby both icons are initial visible (until the JS is triggered)

Note, it might be ideal to de-duplicate these SVGs on the page (see e.g. this stackoverflow), but given these icons are small I don't think this is worth the added complexity for now


part of addressing #1172

@chrisjsewell chrisjsewell requested a review from danwos May 15, 2024 01:13
Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 85.18519% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.21%. Comparing base (dd28cda) to head (4b3eb73).

Files Patch % Lines
sphinx_needs/layout.py 85.18% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1181      +/-   ##
==========================================
- Coverage   86.31%   86.21%   -0.10%     
==========================================
  Files          56       56              
  Lines        6494     6498       +4     
==========================================
- Hits         5605     5602       -3     
- Misses        889      896       +7     
Flag Coverage Δ
pytests 86.21% <85.18%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chrisjsewell
Copy link
Member Author

Another note, https://sphinx-togglebutton.readthedocs.io also directly adds the SVG to the page, they also wrap it in a button rather than a span, which may be better for accessibility, but I won't do that in this PR
https://github.com/executablebooks/sphinx-togglebutton/blob/f468ca6c5fd4caf0085a97f84794d733794f3614/sphinx_togglebutton/_static/togglebutton.js#L28

@chrisjsewell chrisjsewell merged commit 229d2f8 into master May 15, 2024
19 checks passed
@chrisjsewell chrisjsewell deleted the make-collapse-button-svg branch May 15, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants