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

♻️ REFACTOR: Reduce JS/CSS size and improve accessibility #94

Merged
merged 29 commits into from
Jan 24, 2021

Conversation

foster999
Copy link
Collaborator

@foster999 foster999 commented Oct 11, 2020

Removes:

  • semantic-ui assets
  • unused 'sphinx_tabs_nowarn' sphinx option

Changes:

  • tab structure to use tab roles
  • tab label colour, to increase contrast with background
  • assets are now copied across by sphinx rather than explicitly in the extension

Adds:

  • tabindex atributes to allow users to focus and switch tabs using a keyboard (left and right arrows, and enter key to change)
  • ARIA labels for tabs and panels
  • ability to close tabs panel by clicking open tab
  • persistence of last selected group or code tab during the session (when session storage is permitted)
  • margin below images inside tab content

Closes #92, closes #66, closes #87, closes #90 and closes #80, closes #65, closes #93.
It also includes an equivalent of #73

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #94 (7d2a1c5) into master (05ddd2c) will increase coverage by 3.98%.
The diff coverage is 96.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   90.04%   94.02%   +3.98%     
==========================================
  Files           1        1              
  Lines         201      201              
==========================================
+ Hits          181      189       +8     
+ Misses         20       12       -8     
Flag Coverage Δ
pytests 94.02% <96.61%> (+3.98%) ⬆️

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

Impacted Files Coverage Δ
sphinx_tabs/tabs.py 94.02% <96.61%> (+3.98%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05ddd2c...e5feb59. Read the comment docs.

@foster999 foster999 marked this pull request as ready for review October 14, 2020 18:38
@foster999
Copy link
Collaborator Author

foster999 commented Oct 14, 2020

Quite a large change to the underlying HTML here, so wouldn't be surprised if it introduces bugs for some use cases... Though I've trialed it with the demos in the docs and it seems to work well.

Also worth noting that:

  1. Tabs and tab panels will now be focused on clicking, giving them a black outline until another element is focused. This focus highlighting is useful for those navigating by keyboard, so shouldn't be hidden.
  2. Navigating across a tablist using arrow keys doesn't currently work if there are next/previous buttons on the page (i.e. jupyter-books), because of an event listener in sphinx's doctools.js triggers the page to change. I've raised a separate PR to disable this when button elements are focused, like those used for tabs here.

Edit: doctools.js is now overwritten with a minor change, so that arrows do not switch page when a button is focused. This should be removed if sphinx-doc/sphinx#8316 gets merged.

@pradyunsg
Copy link
Member

Ahoy! As a heads up, I won't be properly reviewing this myself anytime soon (context here) but I definitely wanted to drop in and say -- Thanks a lot for picking this up, and as far as I can see, this looks great!

@foster999
Copy link
Collaborator Author

Hey @chrisjsewell, please could you provide a review of this sometime? I appreciate that there's quite a few changes piled in here, but it made sense to include them given the extent of the rewrite.

If there's nothing obviously broken here I'll be around to pick up any bugs I may have thrown in, if released 😅

@Daltz333
Copy link
Collaborator

There seems to be some weird white outline of the tabbed content that just doesn't fit.
image

sphinx_tabs/static/doctools.js Outdated Show resolved Hide resolved
@chrisjsewell
Copy link
Member

chrisjsewell commented Jan 24, 2021

Heya, @Daltz333 I don't want to be too much of a blocker on this package, so I've invited you and @foster999 as maintainers

I've made a quick review comment above also:

It also includes an equivalent of #73

Could you add a note on this to the documentation (https://sphinx-tabs--94.org.readthedocs.build/en/94/#), and in fact it would be good to improve this page with a bit more explanation of the extension, including sphinx configuration options etc

@foster999
Copy link
Collaborator Author

Thanks @chrisjsewell, sounds good to me

I've dropped the sphinx js override and updated the RTD docs to include code examples and describe features. Please could one of you have a quick look at https://sphinx-tabs--94.org.readthedocs.build/en/94/#, then I think it should be good to go 😄

Copy link
Collaborator

@Daltz333 Daltz333 left a comment

Choose a reason for hiding this comment

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

Looks fine to me! I think we should ship it if no other objections!

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

thanks @foster999 looking good, just a minor thing to change

docs/index.rst Outdated Show resolved Hide resolved
@foster999 foster999 changed the title Rewrite to reduce payload size and improve accessibility ♻️ REFACTOR: Reduce JS/CSS size and improve accessibility Jan 24, 2021
@foster999 foster999 merged commit a0af17d into executablebooks:master Jan 24, 2021
@chrisjsewell
Copy link
Member

cheers!

@chrisjsewell
Copy link
Member

I imagine this should be released as v2.0.0

@pradyunsg
Copy link
Member

Hurray! Thanks for doing this folks! ^>^

@foster999
Copy link
Collaborator Author

Cheers for the review both! Shall punch a new release now

@chrisjsewell
Copy link
Member

I assume you know how: https://github.com/executablebooks/.github/blob/master/CONTRIBUTING.md#the-process-of-creating-a-release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants