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

Fix w3c validation warning type style #1551

Merged

Conversation

mdugue
Copy link
Member

@mdugue mdugue commented Feb 24, 2018

Summary

Removes type="text/css" from the <style /> tag, as it causes the warnings

The type attribute for the style element is not needed and should be omitted.

in w3c validator, see https://validator.w3.org/nu/?doc=https%3A%2F%2Fwww.styled-components.com%2F for an example.

Motivated by https://spectrum.chat/?thread=085d48f2-47e2-43ac-8459-500722a0ba3f

Tests looked fine, snapshots changed as expected. I couldn't see any problems with SSR, jest-styled-components or babel-plugin-styled-components.

TBD

Does styled-components support html 4 environments? If so, this PR should be adjusted to add the type attribute only if in html5-"mode"

@mxstbr-bot
Copy link

mxstbr-bot commented Feb 24, 2018

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@mdugue mdugue changed the title Fix w3 validation warning type style Fix w3c validation warning type style Feb 24, 2018
Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

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

This is fine overall but can you explain why it is recommended to leave it out please? I’m not quite aware of the intricacies of having or not having it, but previously I was under the impression that it’s a best practice to specify the type 😅

CHANGELOG.md Outdated
@@ -2,414 +2,416 @@

All notable changes to this project will be documented in this file. If a contribution does not have a mention next to it, [@geelen](https://github.com/geelen) or [@mxstbr](https://github.com/mxstbr) did it.

*The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/).*
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove your formatting from this file please? It’s be great to preserve its previous history and format

Copy link
Member Author

@mdugue mdugue Feb 24, 2018

Choose a reason for hiding this comment

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

sorry, didn't realise , will revert later on

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@quantizor
Copy link
Contributor

quantizor commented Feb 24, 2018 via email

@mdugue
Copy link
Member Author

mdugue commented Feb 24, 2018

@kitten right, its not hurting to include the type attribute, but since HTML5 it default to text/css and w3c validation considers its declaration redundant.

Do we have to consider pre html5 environments?

@kitten
Copy link
Member

kitten commented Feb 24, 2018

I’m a little concerned about IE9, but if this works for it that’d be awesome 😎

@mdugue
Copy link
Member Author

mdugue commented Feb 25, 2018

I tested the sandbox in IE8, which didn't seem to have any Problems 😃

@mdugue
Copy link
Member Author

mdugue commented Feb 25, 2018

Btw. I just checked https://emotion.sh/ out of curiosity. It doesn't seem to set type attributes either

@kitten kitten merged commit 247a5fe into styled-components:master Feb 25, 2018
@mxstbr
Copy link
Member

mxstbr commented Feb 25, 2018

Thank you so much for helping us improve styled-components! Based on our Community Guidelines every person that has a PR of any kind merged is offered an invitation to the styled-components organization—that is you right now!

Should you accept, you'll get write access to the main repository. (and a fancy styled-components logo on your GitHub profile!) You'll be able to label and close issues, merge pull requests, create new branches, etc. We want you to help us out with those things, so don't be scared to do them! Make sure to read our contribution and community guidelines, which explains all of this in more detail. Of course, if you have any questions just let me 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.

None yet

5 participants