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

Add missing single-word attributes to property warning list #10495

Merged
merged 5 commits into from Aug 21, 2017

Conversation

nhunzaker
Copy link
Contributor

I never added the all-lowercase words to the list of attribute casings to warn about. @flarnie caught this when you pass something like SiZe. This is a known HTML attribute. We need to warn users that they should specify this property as size.

Basically I just ran this over the 15.6.1 whitelist:

Object.keys(Properties)
      .filter(key => key === key.toLowerCase())
      .map(key => `${key}: '${key}',`).join('\n')

And pasted it over to the list.

color: 'color',
results: 'results',
security: 'security',
unselectable: 'unselectable',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we start from A three times? Are these generated from different whitelists? In this case should there be a comment explaining grouping
Or we could sort them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's a combination of the HTML Property Config and SVG Property Config, which where ultimately just blended into the same list inside of DOMProperty.properties

Wrestled with some editor-fu, but I've alphabetized these while still clumping together aliases.

@gaearon
Copy link
Collaborator

gaearon commented Aug 18, 2017

Accepted with nit (unclear sort order). Let's leave to @flarnie for second review.

@sophiebits
Copy link
Collaborator

Mind adding a test? A simple one is fine.

@nhunzaker
Copy link
Contributor Author

@spicyj added a very basic test in de06c4a

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

loveit

@gaearon
Copy link
Collaborator

gaearon commented Aug 18, 2017

If we later add an attribute to whitelist should we also add it to this DEV whitelist?

@nhunzaker
Copy link
Contributor Author

Yes. We could seed the list with existing property names from the whitelist, but i thought this was much simpler, even if duplicative

@gaearon
Copy link
Collaborator

gaearon commented Aug 18, 2017

Worth adding a comment to both maybe to make this clearer.

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Aug 18, 2017 via email

@flarnie
Copy link
Contributor

flarnie commented Aug 21, 2017

Thanks for doing this! I liked the helpful comments - I think this makes sense.

I'm going to do my testing work on top of this branch, assuming you will merge whenever you get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants