Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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.

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

Successfully merging this pull request may close these issues.

5 participants