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

Reposition the admonition title icon to the top of the paragraph in m… #2100

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stevepiercy
Copy link
Contributor

…ulti-line titles

Fixes #2099

Copy link

github-actions bot commented Jan 20, 2025

Coverage report

This PR does not seem to contain any modification to coverable code.

@stevepiercy
Copy link
Contributor Author

See #2101 for RTD build failure issue.

drammock
drammock previously approved these changes Jan 28, 2025
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

this LGTM, ping @gabalafou for affirmation that hard coding 0.5rem is the right call here (IDK if we have a variable we should use instead, or if there's a cleverer way to align this icon to the top text line)

@gabalafou
Copy link
Collaborator

@drammock, yes it would be better to make the admonition title bar a flex box and to align the icon with the text that way - i.e.:

display: flex;
flex-direction: row;
align-content: baseline;

But to do that we would need to refactor the admonition CSS. This is probably something we should do because right now I think that it's a bit overly complicated. For example, the container has horizontal padding that the title element has to offset with negative margins. That suggests to me that there's almost certainly a cleaner, simpler way to structure our admonition styles. But that's a bigger, longer fix.

For now, setting the top property to .4rem (it should be .4 not .5rem) works because it matches the value of the top padding. So what I've done is pushed a commit to make these alignment relationships explicit by using SASS variables.

@drammock
Copy link
Collaborator

a11y failures seem unrelated, but happened again after restarting the job, so seems like maybe not a transient network problem:

         # On the PyData Library Styles page, wait for ipywidget to load and for our
        # JavaScript to apply tabindex="0" before running Axe checker (to avoid
        # false positives for scrollable-region-focusable).
        if url_pathname == "/examples/pydata.html":
            ipywidgets_pandas_table = page.locator("css=.jp-RenderedHTMLCommon").first
>           expect(ipywidgets_pandas_table).to_have_attribute("tabindex", "0")
E           AssertionError: Locator expected to have attribute '0'
E           Actual value: <element(s) not found> 
E           Call log:
E             - LocatorAssertions.to_have_attribute with timeout 5000ms
E             -   - waiting for locator(".jp-RenderedHTMLCommon").first

tests/test_a11y.py:178: AssertionError

and

 E           playwright._impl._errors.TimeoutError: Locator.wait_for: Timeout 30000ms exceeded.
E           Call log:
E             - waiting for locator(".jp-RenderedHTMLCommon").first

.tox/a11y-tests/lib/python3.12/site-packages/playwright/_impl/_connection.py:528: TimeoutError

@trallard what is your feeling about merging with red CIs? In my other projects I'd pause this PR until the CIs are fixed elsewhere, then merge main into this one to get CIs green. But approaches differ and I forget if we have a policy.

@gabalafou
Copy link
Collaborator

I just ran the accessibility tests locally (tox run -e a11y-tests) and they passed, just so you know

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.

Multi-line admonition title's icon aligns with last line
3 participants