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 #624

Closed
wants to merge 3 commits into from
Closed

Remove Modernizr #624

wants to merge 3 commits into from

Conversation

jessetan
Copy link
Contributor

@jessetan jessetan commented May 1, 2018

DO NOT MERGE YET

This PR removes Modernizr, as discussed in #525.
Behavior of non-evergreen browsers (mainly IE6 - IE11) needs to be verified, as this also removes the html5shiv that is incorporated in Modernizr. Possible effects may be incorrect rendering, buttons not working, navigation not functioning, JavaScript errors in console

This PR does not remove the rAF polyfill for old IE.

  • Chrome (latest) verified
  • Firefox (latest) verified
  • Edge (latest) verified
  • IE11 verified
  • IE10 verified
  • IE9 verified
  • IE8 verified
  • IE7 verified
  • IE6 verified

@jessetan
Copy link
Contributor Author

jessetan commented May 1, 2018

IE 10 on Windows 7 looks broken with and without Modernizr:

ie10-win7

This is not visible on Microsofts Browser screenshot page.

@mitya57
Copy link
Contributor

mitya57 commented Jul 7, 2018

Microsoft has a great service for generating website screenshots with different browsers, including all IE versions since IE 8:

https://developer.microsoft.com/en-us/microsoft-edge/tools/screenshots/

If you deploy your changes somewhere, it will be quite easy to see which browsers get broken.

@jessetan
Copy link
Contributor Author

jessetan commented Jul 9, 2018

I know of that page (I even referenced it in the comment above) but it seems to produce some different results from actual tests on browsers. Since I don't know exactly how they are creating the screenshots, I prefer manual testing for such an important change.
Screenshots also don't show JS errors, scrolling behavior, click behavior etc.

@mitya57
Copy link
Contributor

mitya57 commented Jul 10, 2018

Oh, how could I not notice that link :)

@rupl
Copy link

rupl commented Jul 18, 2018

Hi just my 2¢ as the Modernizr module maintainer, you should definitely not hardcode Modernizr into your theme and instead provide any Modernizr tests you need using either a hook or theme.info entry.

That way people can upgrade Modernizr on their own schedule, other modules can also control what tests are included, and you have less to maintain.

@jessetan
Copy link
Contributor Author

@rupl thanks for commenting. I'm not sure how applicable your comment is, since sphinx_rtd_theme is not a Drupal theme, but a Sphinx theme.

@rupl
Copy link

rupl commented Jul 18, 2018

looooool sorry 😳

@jessetan jessetan mentioned this pull request Feb 20, 2019
@Blendify
Copy link
Member

I think we should go forward with this.

@jessetan
Copy link
Contributor Author

Closing in favor of #756

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.

4 participants