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

change allowTransparency to allowtransparency #1538

Merged
merged 1 commit into from Dec 13, 2019

Conversation

@dawidvdh
Copy link
Contributor

dawidvdh commented Nov 15, 2017

Changed allowTransparencyto allowtransparency to account for the following react error:

Warning: React does not recognize the allowTransparency prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase allowtransparency instead. If you accidentally passed it from a parent component, remove it from the DOM element.

react - 16.1.1
react-dom - 16.1.1

…t >= v16.1
@@ -108,7 +108,7 @@ const SVGDOM_ATTRIBUTE_NAMES = {

const DOM_PROPERTY_NAMES = [
// Standard
'acceptCharset', 'accessKey', 'allowFullScreen', 'allowTransparency', 'autoComplete', 'autoFocus', 'autoPlay',
'acceptCharset', 'accessKey', 'allowFullScreen', 'allowtransparency', 'autoComplete', 'autoFocus', 'autoPlay',

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 15, 2017

Collaborator

Did react remove support for this property? I’m reasonably sure it was supported at the time this list was created. What version dropped support for it?

The lowercase one is just a generic custom property name; there’s no need to include it in the list at all.

This comment has been minimized.

Copy link
@jomasti

jomasti Nov 16, 2017

Contributor

Looks like it was removed in facebook/react#10823, which was released in 16.1.0. It must have been missed when making the release notes.

@@ -108,7 +108,7 @@ const SVGDOM_ATTRIBUTE_NAMES = {

const DOM_PROPERTY_NAMES = [
// Standard
'acceptCharset', 'accessKey', 'allowFullScreen', 'allowTransparency', 'autoComplete', 'autoFocus', 'autoPlay',

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 16, 2017

Collaborator

In that case, it should be present unless the React version declared in the settings is 16.1+.

@gaearon
Copy link
Collaborator

gaearon commented Nov 16, 2017

Can you completely remove it? That's what React did. This attribute only works for IE8 and lower. React stopped supporting them more than a year ago.

@ljharb
Copy link
Collaborator

ljharb commented Nov 16, 2017

We should be supporting what is, not what the documentation claims things are. It seems like the attribute arrives in the HTML in every browser prior to React 16.1; whether it has functionality or not isn't particularly relevant.

@yannickcr yannickcr force-pushed the yannickcr:master branch from dca5cd5 to c148893 Nov 18, 2017
@ljharb ljharb force-pushed the dawidvdh:fix/allowTransparency branch from beaaf69 to c933a19 Dec 13, 2019
@ljharb
Copy link
Collaborator

ljharb commented Dec 13, 2019

I rebased this and added the version logic.

@ljharb
ljharb approved these changes Dec 13, 2019
@ljharb ljharb force-pushed the dawidvdh:fix/allowTransparency branch 2 times, most recently from 19aa5b7 to fd3cebb Dec 13, 2019
@ljharb ljharb merged commit fd3cebb into yannickcr:master Dec 13, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 97.509%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.