Add SVG attributes #465
Merged
Add SVG attributes #465
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SVG attributes like "d" would not be passed to components, making SVG elements not render correctly (or at all). This commit adds SVG attributes as described on MDN (https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute) to the whitelist that are passed down to wrapped components. closes emotion-js#357
emmatown
reviewed
Nov 17, 2017
| @@ -1,6 +1,6 @@ | |||
| import React from 'react' | |||
| import renderer from 'react-test-renderer' | |||
| import styled, { css, flush } from 'react-emotion' | |||
| import styled, { css, flush } from '../src/' | |||
There was a problem hiding this comment.
Is there a reason this was changed? We alias all the packages to their src and import them by name so we can runs tests on the dist files.
emmatown
reviewed
Nov 21, 2017
There was a problem hiding this comment.
It looks like some snapshots are failing, could you update them?
emmatown
approved these changes
Nov 21, 2017
tkh44
approved these changes
Nov 21, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What: This PR adds SVG attributes listed on MDN's SVG Attribute Reference page to the attribute whitelist so they are successfully passed to SVG elements like
pathandcircle. Omissions are onClick and friends andfran attribute onradialGradientelements which React seems to not seem to view as a legitimate attribute (it warns that it must be removed).Why: Currently key attributes required for the element to be rendered/visible are stripped off resulting in
pathelements not displaying on screen.How:
Checklist:
I'm happy to add documentation for this, but essentially this removes surprising behaviour and makes SVG work just like the rest of the documented API. As such, documenting the absence of this would probably provide more value than documenting it's presence.
It there are improvements in the tests or code that can should be made, please let me know. This is currently blocking me from adopting Emotion in one of my projects that uses a bunch of SVG so I'm keen to get merge-worthy.
Additionally, this closes #357
One other note: One of the commits here updates some of the existing snapshots. While I am a fan of Jest snapshotting failures aren't super instructive. Basically scale and fontSize attributes showed up in a few tests. I thought I would flag that so it get's a careful look and the implications could be thought about by people who are deeper into this than I am.