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

Remove Modernizr, keep html5shiv (Webpack edition) #848

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@jessetan
Copy link
Contributor

jessetan commented Oct 25, 2019

Modernizr has been part of the theme for a long time, although it is not used. Since its presence has downsides (reflow on load, increased theme size), we want to remove it.
To keep backward compatibility with older IEs, I've kept html5shiv in this PR so there is less chance of breakage.

This is a rework of #756 after our move to Webpack and friends. Rebasing did not make much sense.
Fixes #724, #525. Related #414. Supersedes #624 and #756

@jessetan jessetan requested a review from readthedocs/theme-committers Oct 25, 2019
Copy link
Collaborator

agjohnson left a comment

Change looks good to me!

I know this has been a long standing issue and we've been slow to adopt the change, so I don't want to keep holding up the work here. Perhaps just some screengrabs from older IE to verify that the experience isn't completely broken without modernizer?

sphinx_rtd_theme/layout.html Show resolved Hide resolved
@jessetan

This comment has been minimized.

Copy link
Contributor Author

jessetan commented Oct 26, 2019

Any particular IE versions you want to see? Microsoft no longer offers free screenshots using Browserstack, that requires a paid account.
They do offer VMs with IE8 and up.

@agjohnson

This comment has been minimized.

Copy link
Collaborator

agjohnson commented Nov 1, 2019

I tried saucelabs, but they dropped IE8 support. IE9 looked as expected though. I tried a demo account on browserstack, but couldn't test IE8. I even tried ie8 through wine to no avail.

I guess I'm not sure how thorough we need to be here then, testing sounds annoying. IE8 only accounts for 0.1% of RTD traffic, which also isn't 100% for projects using this theme.

@jessetan

This comment has been minimized.

Copy link
Contributor Author

jessetan commented Nov 8, 2019

Perhaps the best way to get feedback on this is to release a dev version of the theme and to wait for user responses or bug reports.

@agjohnson

This comment has been minimized.

Copy link
Collaborator

agjohnson commented Nov 8, 2019

Sounds good.

@agjohnson agjohnson merged commit 2b8717a into master Nov 8, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@agjohnson agjohnson deleted the html5shivwebpack branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.