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

Better support for HTML5 attributes #102

Merged
merged 18 commits into from Sep 26, 2016
Merged

Better support for HTML5 attributes #102

merged 18 commits into from Sep 26, 2016

Conversation

gcacars
Copy link
Contributor

@gcacars gcacars commented Sep 17, 2016

Better support for HTML5 attributes in Spacebars helpers. Before HTML5 boolean attributes is not handled properly.
Fixes: #52

Better support for HTML5 attributes in Spacebars Helpers. Before HTML5 is not handled properly.
@apollo-cla
Copy link

apollo-cla commented Sep 17, 2016

@gcacars: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

return new BooleanHandler(name, value);
} else if (name === 'spellcheck' && elem.contentEditable === 'true' ||
(elem.tagName === 'TEXTAREA' ||
(elem.tagName === 'INPUT' && elem.type !== 'password'))) {
Copy link
Collaborator

@mitar mitar Sep 17, 2016

Choose a reason for hiding this comment

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

Hm, I think in our code style we are aligning all those (?

Can you improve order/readability/add more parenthesis here because it is unclear how this conditions combine?

Copy link
Contributor Author

@gcacars gcacars Sep 17, 2016

Choose a reason for hiding this comment

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

Changed. This way is better?

@mitar
Copy link
Collaborator

mitar commented Sep 17, 2016

Can you please also add some tests for these changes?

return new BooleanHandler(name, value);
} else if ((name === 'spellcheck' && elem.contentEditable === 'true') ||
(name === 'spellcheck' && elem.tagName === 'TEXTAREA') ||
(name === 'spellcheck' && elem.tagName === 'INPUT' && elem.type !== 'password')) {
Copy link
Collaborator

@mitar mitar Sep 17, 2016

Choose a reason for hiding this comment

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

Hm, maybe:

(name === 'spellcheck' && (elem.contentEditable === 'true' || elem.tagName === 'TEXTAREA' || (elem.tagName === 'INPUT' && elem.type !== 'password'))

Copy link
Contributor Author

@gcacars gcacars Sep 17, 2016

Choose a reason for hiding this comment

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

Hmmm... right, I understood now! contenteditable was wrong before. Changed.

name === 'controls' || name === 'loop' || name === 'muted')) {
return new BooleanHandler(name, value);
} else if (name === 'spellcheck' &&
(elem.contentEditable === 'true' || elem.tagName === 'TEXTAREA' ||
Copy link
Collaborator

@mitar mitar Sep 17, 2016

Choose a reason for hiding this comment

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

Indent this for one space.

Copy link
Contributor Author

@gcacars gcacars Sep 18, 2016

Choose a reason for hiding this comment

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

Changed. I did this to be clear that a parenthesis is inside another.

Copy link
Collaborator

@mitar mitar Sep 18, 2016

Choose a reason for hiding this comment

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

Hm, I thought you will add one space, not remove it. ;-)

Copy link
Collaborator

@mitar mitar Sep 18, 2016

Choose a reason for hiding this comment

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

So that second and third are indented once, but at the same level.

Copy link
Contributor Author

@gcacars gcacars Sep 18, 2016

Choose a reason for hiding this comment

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

Sorry, I had not understood. Now is correct?

@mitar
Copy link
Collaborator

mitar commented Sep 17, 2016

Also, add tests please.

@gcacars
Copy link
Contributor Author

gcacars commented Sep 18, 2016

I have no idea which tests you need. Can you help me with this?

@mitar
Copy link
Collaborator

mitar commented Sep 18, 2016

Tests. Something like this, just for boolean variables. So you would like to make sure some of those variables are rendered as boolean variables.

- Alphabetic order for constructors of HTML elements
- Issue meteor#52, PR meteor#102
@mitar
Copy link
Collaborator

mitar commented Sep 18, 2016

Tests failed on CircleCI. I am rerunning them.

gcacars added 2 commits Sep 18, 2016
- Removed autocomplete (not a boolean attribute)
- Fix names of DOM properties
@mitar
Copy link
Collaborator

mitar commented Sep 18, 2016

Hm, why those attributes have to be camel case?

gcacars added 2 commits Sep 18, 2016
- Spellcheck attribute is a boolean, but is handled like strings.
- Fix iframe sandbox with others values
- Removed <script> async
- Spellcheck attribute is a boolean, but is handled like strings.
- Removed <script> async
@mitar
Copy link
Collaborator

mitar commented Sep 18, 2016

Can you explain why removing those?

@gcacars
Copy link
Contributor Author

gcacars commented Sep 18, 2016

Fixed some bugs. I removed that script async for now. I don't know what happened with him.
Spellcheck has values "true" and "false", is not a trully boolean

speelcheck="true"

@mitar
Copy link
Collaborator

mitar commented Sep 18, 2016

Also, add a line to HISTORY file explaining this pull request. So that once it is merged, history will be up-to-date. See example of split templating package for the format of the line I think suitable for this pull request as well.

@mitar
Copy link
Collaborator

mitar commented Sep 18, 2016

Good that we have tests. :-) Please add tests for all those cases you found as tricky.

@mitar
Copy link
Collaborator

mitar commented Sep 18, 2016

Tell me when you think this is ready and complete.

@gcacars
Copy link
Contributor Author

gcacars commented Sep 26, 2016

@mitar for me, it's all ok.

@gcacars gcacars closed this Sep 26, 2016
@gcacars gcacars reopened this Sep 26, 2016
@mitar mitar merged commit e9959eb into meteor:master Sep 26, 2016
2 checks passed
@mitar
Copy link
Collaborator

mitar commented Sep 26, 2016

Thanks!

} else if (elem.tagName === 'FORM' && name === 'novalidate') {
return new BooleanHandler(name, value);
} else if (elem.tagName === 'IFRAME' && name === 'sandbox' && !value) {
// Disable sandbox mode (others values will be handled as a normal attr)
Copy link
Collaborator

@mitar mitar Dec 30, 2016

Choose a reason for hiding this comment

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

@gcacars: Why is sandbox handled differently?

Copy link
Contributor Author

@gcacars gcacars Dec 31, 2016

Choose a reason for hiding this comment

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

Because we can have:
<iframe sandbox>
And this way, the attribute is simply true or false.
Or we can have this others values:

<iframe sandbox="allow-forms">
<iframe sandbox="allow-pointer-lock">

mitar added a commit that referenced this issue Dec 31, 2016
Reverted #102 and made a different fix for removing attributes for
`false` values in dynamic attributes (#52). Fixes #151.
@mitar
Copy link
Collaborator

mitar commented Dec 31, 2016

A different fix has been implemented in 84c58a6.

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.

None yet

3 participants