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

Create sanitized attribute and property classes #197

Closed
wants to merge 2 commits into from

Conversation

zackthehuman
Copy link
Contributor

@zackthehuman zackthehuman commented Jun 22, 2016

let { element } = this;
let { tagName } = element;

if (SanitizedProperty.requiresSanitization(tagName, name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this use the method off of SanitizedNonNamespacedAttribute instead?

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, this is the result of copy pasta. Will fix.

} else if (tagName === 'INPUT' && name === 'value') {
this.group.push(new EmptyStringResetingProperty(element, name, reference));
} else {
this.group.push(new NonNamespacedAttribute(element, name, reference));
}
Copy link
Contributor Author

@zackthehuman zackthehuman Jun 23, 2016

Choose a reason for hiding this comment

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

I changed the conditions here. This used to push two objects in the case of input[value]. Was this the correct behavior? For the sanitized case it should be one or the other, not both. @asakusuma

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I would have thought the tests would fail if you did this but apparently not. I had assumed you needed both. I added a specific test case around this though, so I'm not too worried about this change breaking things.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

@krisselden
Copy link
Contributor

This was superseded

@krisselden krisselden closed this Jul 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants