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

Align prev/next without pseudo-element #1152

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jonels-msft
Copy link
Contributor

Fixes #951

Slightly uglier HTML, and an asymmetry in the float attribute between buttons, but should remove an extra stop in a screenreader.

@stsewd stsewd requested a review from agjohnson May 26, 2021 14:05
@stsewd
Copy link
Member

stsewd commented May 26, 2021

From the PR preview, it looks like this, I think maybe because you need to re-generate the assets? https://sphinx-rtd-theme.readthedocs.io/en/latest/contributing.html#making-changes (npm run build)

Screenshot_2021-05-26 Installation — Read the Docs Sphinx Theme 0 5 2 documentation

@jonels-msft
Copy link
Contributor Author

@stsewd hm, I did view it locally with npm run dev and resized the window to test how it behaved. Maybe it's a browser difference. I tested on Chrome 90.

I'll try floating the left button too, since looks like it's clearing the other button.

@stsewd
Copy link
Member

stsewd commented May 26, 2021

@stsewd hm, I did view it locally with npm run dev and resized the window to test how it behaved. Maybe it's a browser difference. I tested on Chrome 90.

I'll try floating the left button too, since looks like it's clearing the other button.

I meant that you need to commit the changed files from npm run build to be able to review it from the preview https://sphinx-rtd-theme--1152.org.readthedocs.build/en/1152/

@jonels-msft jonels-msft marked this pull request as draft May 26, 2021 17:23
@Blendify
Copy link
Member

My main concern here is flex compatibility issues with older browsers. I am working on a auto prefixer to add browser css extensions automatically. Once I get that patch in we can move forward with using flex in more places.

@jonels-msft
Copy link
Contributor Author

jonels-msft commented Jun 1, 2021

@Blendify fair point about compatibility. How far back are we looking at for browser support?
If I'm reading it right, https://caniuse.com/flexbox shows full support in firefox 28 (seven years ago), and chrome 21 (nine years ago).

Some context behind this PR. The linked bug in the description is part of a set of accessibility issues that our organization must fix by July 1 in our docs system. What's the timeline for your auto-prefixer patch? Is there a shim I can use for flexbox in the meantime?

@Blendify
Copy link
Member

Is there a shim I can use for flexbox in the meantime?

Yes, using display: -ms-flexbox should be sufficient.

@jonels-msft
Copy link
Contributor Author

@Blendify thanks for the tip! How does this look?

@Blendify
Copy link
Member

This blog can explain it better than I can quickly: https://www.thoughtco.com/css-vendor-prefixes-3466867

@Blendify
Copy link
Member

@agjohnson can you test this on older IE browsers?

@Blendify Blendify marked this pull request as ready for review June 18, 2021 15:07
@Blendify Blendify added this to the 1.1 milestone Aug 6, 2021
@benjaoming benjaoming modified the milestones: 1.1, 2.0 Aug 24, 2022
@benjaoming
Copy link
Contributor

I've pushed this to 2.0 - in this release, we could drop old browser support and merge this PR without having to test it :)

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.

Too many keyboard focus targets on Next and Previous buttons
4 participants