X Tutup
Skip to content

fix(HtmlParser): mark <source> elements as void#5668

Closed
pkozlowski-opensource wants to merge 1 commit intoangular:masterfrom
pkozlowski-opensource:source_void
Closed

fix(HtmlParser): mark <source> elements as void#5668
pkozlowski-opensource wants to merge 1 commit intoangular:masterfrom
pkozlowski-opensource:source_void

Conversation

@pkozlowski-opensource
Copy link
Copy Markdown
Member

@pkozlowski-opensource pkozlowski-opensource commented Dec 7, 2015

Fixes #5663

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Dec 7, 2015

@vicb picked up this one to get (a bit) familiar with the new HTML parser code. Not sure what you think about having tests for each specific tags

@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 7, 2015
@vicb
Copy link
Copy Markdown
Contributor

vicb commented Dec 7, 2015

Not sure what you think about having tests for each specific tags

Not sure if it would be really useful - if you write the source and the tests, they would probably pass. however it doesn't hurt either.

There are also more void elements that are not currently covered, could you add them ? (http://www.w3.org/TR/html5/syntax.html#void-elements)

@vicb vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 7, 2015
vicb
vicb reviewed Dec 7, 2015
View reviewed changes
modules/angular2/src/compiler/html_tags.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 7, 2015

Choose a reason for hiding this comment

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

requiredParents means that the first parent (audio) will be automatically added when none of them is present. I don't think it's like that in the spec, is it ?

@pkozlowski-opensource pkozlowski-opensource removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 7, 2015
@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Dec 7, 2015

@vicb updated. Mind having another look?

vicb
vicb reviewed Dec 7, 2015
View reviewed changes
modules/angular2/test/compiler/html_parser_spec.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 7, 2015

Choose a reason for hiding this comment

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

[
  'fix1',
  'fix2',
].forEach((html) => {
  expect(parser.parse(html, 'TestComp').errors).toEqual([]);
});

Copy link
Copy Markdown
Member Author

@pkozlowski-opensource pkozlowski-opensource Dec 7, 2015

Choose a reason for hiding this comment

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

👍

vicb
vicb reviewed Dec 7, 2015
View reviewed changes
modules/angular2/test/compiler/html_parser_spec.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 7, 2015

Choose a reason for hiding this comment

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

nits:

  • remove the extra blank lines
  • have one fixture per line (I think that if you add a "," after the last element clang will not mess with the formatting)
  • close the .forEach() with a ;

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Dec 7, 2015

@vicb clang-format seems to have very strong opinion about how things should work.... I'm not sure I find it easier to read now. But hey, this is why we've got formatter to make us all agree :-)

Feel free to fiddle more with the formatting if you've got an idea on making it look nicer

vicb
vicb reviewed Dec 7, 2015
View reviewed changes
modules/angular2/test/compiler/html_parser_spec.ts Outdated
Copy link
Copy Markdown
Contributor

@vicb vicb Dec 7, 2015

Choose a reason for hiding this comment

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

});}); missing ";"

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Dec 8, 2015

LGTM

@vicb vicb added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 8, 2015
@vicb vicb added this to the beta.0 milestone Dec 8, 2015
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Dec 8, 2015

Merging PR #5668 on behalf of @jelbourn to branch presubmit-jelbourn-pr-5668.

@vicb vicb mentioned this pull request Dec 8, 2015
Closed
@robwormald robwormald mentioned this pull request Dec 8, 2015
Closed
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 7, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@vicb vicb

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Milestone

beta.0

Development

Successfully merging this pull request may close these issues.

<source> tag throwing error since alpha.48 (d388c0a)

5 participants

@pkozlowski-opensource @vicb @mary-poppins @jelbourn @googlebot
X Tutup