-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Use semantic markup in flyout #1164
base: master
Are you sure you want to change the base?
Use semantic markup in flyout #1164
Conversation
src/sass/_theme_badge.sass
Outdated
@@ -85,6 +85,9 @@ | |||
padding: 0 $base-line-height / 4 | |||
display: block | |||
text-align: center | |||
h3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use a more specific CSS selector here such as a "label" class.
1950e65
to
bb7ecf4
Compare
@Blendify I added the label to html and style. See if it looks good. |
Looks good but I want confirmation from @agjohnson that this won't have any issue with read the docs itself |
Change looks good, this doesn't break rtd.org, but it won't be visible in projects from .org, since we override that section with https://github.com/readthedocs/readthedocs.org/blob/master/readthedocs/api/v2/templates/restapi/footer.html. Having that change in the repo I just linked would be ideal, but it may break users using some customization around, since it's changing the tags. We are working to return a json with the data instead of html so themes can customize this better. |
@stsewd would you consider, as an intermediate step between the current situation and the JSON footer, having a flag in RTD to toggle the markup in restapi/footer.html? By default it could stay what it is, but through a configuration parameter it could switch to being more accessible? |
Yea, I think we could improve the content on the RTD-injected footer, and set it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get a chance to take a deep look at this, but this is a strong breaking change for downstream usage of the theme. Many of our users and customers have worked around this CSS to hide these elements unfortunately, so it makes this change pretty costly.
If you need this in your docs immediately, it might be best as a local override of the theme in your own theme.
We talked about this internally; I didn't realize this was changing the structure of the footer. That's a much larger change, and something that will effect a lot of downstream users, so it's something that we'll need to do a lot more work to support, at least on the RTD side of things. If there's a way to get more accessible options for the existing markup, that's something we could push out quickly -- but changing the actual HTML is going to be much harder. We're actually going to be moving to a data-driven approach to the footer, which will avoid this problem (readthedocs/readthedocs.org#8052) -- but that work isn't on our near-term roadmap currently. |
If I'm understanding the situation correctly, it won't be possible to fix this accessibility problem on RTD-hosted sites until readthedocs/readthedocs.org#8052 goes live? (Unless there's a way to decorate the existing markup without substantially changing it.) That is, if I cherry-pick this PR to my fork of the theme and use the forked theme in https://docs.citusdata.com , will RTD still override my changes with the |
@jonels-msft Yeah, that's mostly correct. We do need to design and develop and API return for themes to consume first. The issue is that we can't easily change the DOM structure injected by our application because downstream users and customers use custom stylesheets based on the existing DOM structure output from our application. The part that makes this expensive is we would need to support multiple DOM structures from our application. Instead, we have decided a while back that we want to remove generation of this DOM structure from our application entirely, so this would be going in the opposite direction. It is however possible to hard code a That is, if you are using a derivative theme of sphinx_rtd_theme, our client will replace |
Fixes #953
I've always had a bit of a soft spot for
<dl>
, but in the case of our flyout menu it confuses screen readers. Semantically what we have is a set of headers describing lists of items, so that's the markup I used.