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

make code-block focusable #1104

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/pydata_sphinx_theme/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -923,7 +923,7 @@ def _overwrite_pygments_css(app, exception=None):


# ------------------------------------------------------------------------------
# customize rendering of the links
# customize rendering of tag elements
# ------------------------------------------------------------------------------


Expand Down
32 changes: 30 additions & 2 deletions src/pydata_sphinx_theme/bootstrap_html_translator.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from sphinx.writers.html5 import HTML5Translator
from sphinx.util import logging
from sphinx.ext.autosummary import autosummary_table
from docutils import nodes

logger = logging.getLogger(__name__)

Expand All @@ -22,12 +23,30 @@
self.settings.table_style = "table"

def starttag(self, *args, **kwargs):
"""ensure an aria-level is set for any heading role"""
"""
Perform small modification on specific tags:
- ensure an aria-level is set for any heading role
- include the pst_atts in the final attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a quick note of why atts and class are added / what they do?

- include the pst_class in the final classes
"""
# update attributes using pst_atts
if hasattr(self, "pst_atts"):
for att, val in self.pst_atts.items():
kwargs[att] = kwargs.pop(att, val)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the same as:

if att not in kwargs:
    kwargs[att] = val

?

If so, I feel like that is a bit easier to understand. But maybe I am mis-reading these lines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not exactly equivalent. here what I do is :
try to find the attribute: yes: pop it else use our value

In yours you always replace it even if it's already in kwargs

Copy link
Collaborator

Choose a reason for hiding this comment

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

not exactly equivalent. here what I do is : try to find the attribute: yes: pop it else use our value

In yours you always replace it even if it's already in kwargs

No, check the if condition. (Aside: another way to do this that avoids the conditional altogether is to use dict.setdefault)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad you are right. I'll have a look to setdefault I want to avoid "if" as much as possible

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do find setdefault to be easier to quickly understand. Personally I find the "if" statement to be the least "idiosyncratic" and most straightforward to understand, but setdefault works too.


# update classes using pst_class
if hasattr(self, "pst_class"):
existing_classes = kwargs.get("CLASS", "").split(" ")
classes = list(set(existing_classes + self.pst_class))
kwargs["CLASS"] = " ".join(classes)

Check warning on line 41 in src/pydata_sphinx_theme/bootstrap_html_translator.py

View check run for this annotation

Codecov / codecov/patch

src/pydata_sphinx_theme/bootstrap_html_translator.py#L39-L41

Added lines #L39 - L41 were not covered by tests

# ensure an aria-level is set for any heading role
if kwargs.get("ROLE") == "heading" and "ARIA-LEVEL" not in kwargs:
kwargs["ARIA-LEVEL"] = "2"

return super().starttag(*args, **kwargs)

def visit_table(self, node):
def visit_table(self, node: nodes.Element) -> None:
"""
copy of sphinx source to *not* add 'docutils' and 'align-default' classes
but add 'table' class
Expand Down Expand Up @@ -62,3 +81,12 @@

tag = self.starttag(node, "table", CLASS=" ".join(classes), **atts)
self.body.append(tag)

def visit_literal_block(self, node: nodes.Element) -> None:
"""overwrite the literal-block element to make them focusable"""

# add tabindex to the node
pst_atts = getattr(node, "pst_atts", {})
Copy link
Collaborator

Choose a reason for hiding this comment

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

so pst_atts are chunks of data we are adding to the object on our own in order to re-use them later? And I guess we can't just modify atts and classes directly?

Copy link
Collaborator Author

@12rambau 12rambau Jan 9, 2023

Choose a reason for hiding this comment

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

classes and atts are keyword arguments of the starttag method. They are not reachable without a strong override of the visit method (as I did in my first proposal). In this case the advantage is to carry the information within the node object allowing me to make very small changes to visit methods:

def visit_xxx(self, node):

    node.pst_class = "toto tutu tata"

    super().visit_xxx(node)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah wait so pst is not short for "pydata sphinx theme"? These are in-built sphinx things?

Copy link
Collaborator Author

@12rambau 12rambau Jan 9, 2023

Choose a reason for hiding this comment

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

you are very right "pst" stands for pydata-sphinx-theme which guarantee that no other lib should interfere with these values.

It's the cool thing about Python I can add as many member as I want even after init.

self.pst_atts = {**pst_atts, "tabindex": "0"}

return super().visit_literal_block(node)