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.

Sign up for GitHub

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

Jump to bottom

Make attribute "charset" valid #1863

Merged
merged 1 commit into from Jun 30, 2018
Merged

Make attribute "charset" valid #1863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rules/no-unknown-property.js
View file
Open in desktop
Expand Up @@ -117,7 +117,7 @@ const SVGDOM_ATTRIBUTE_NAMES = {
const DOM_PROPERTY_NAMES = [
// Standard
'acceptCharset', 'accessKey', 'allowFullScreen', 'allowTransparency', 'autoComplete', 'autoFocus', 'autoPlay',
'cellPadding', 'cellSpacing', 'charSet', 'classID', 'className', 'colSpan', 'contentEditable', 'contextMenu',
'cellPadding', 'cellSpacing', 'classID', 'className', 'colSpan', 'contentEditable', 'contextMenu',
'dateTime', 'encType', 'formAction', 'formEncType', 'formMethod', 'formNoValidate', 'formTarget',
'frameBorder', 'hrefLang', 'htmlFor', 'httpEquiv', 'inputMode', 'keyParams', 'keyType', 'marginHeight', 'marginWidth',
'maxLength', 'mediaGroup', 'minLength', 'noValidate', 'onAnimationEnd', 'onAnimationIteration', 'onAnimationStart',
Expand Down
2 changes: 2 additions & 0 deletions tests/lib/rules/no-unknown-property.js
View file
Open in desktop
Expand Up @@ -30,6 +30,8 @@ ruleTester.run('no-unknown-property', rule, {
{code: '<App class="bar" />;'},
{code: '<App for="bar" />;'},
{code: '<App accept-charset="bar" />;'},
{code: '<meta charset="utf-8" />;'},
{code: '<meta charSet="utf-8" />;'},
Copy link
Member

Choose a reason for hiding this comment

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

i think we should be forcing charset; HTML also supports CHARSET and cHaRsEt, but lowercase is the very dominant convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial thought, but charSet is not an unknown attribute, as the name of the rule implies. That said, I’ll modify the PR, so you decide. 😉

Copy link
Member

Choose a reason for hiding this comment

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

hmm, that is true. I suppose it’s a slight overreach of this rule to force it.

Copy link
Contributor Author

@silvenon silvenon Jun 30, 2018 •
edited

Choose a reason for hiding this comment

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

Yeah. As much as I would like for people to write charset instead of charSet, raising an error because of that would not be symmetrical with raising an error for acceptcharset in favor of acceptCharset, because only the latter results into the correct form of the attribute – accept-charset.

I'll hold with the changes then until we agree what they should be.

Copy link
Member

Choose a reason for hiding this comment

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

no need, i think this makes sense as-is

{code: '<App http-equiv="bar" />;'},
{code: '<App xlink:href="bar" />;'},
{code: '<App clip-path="bar" />;'},
Expand Down