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

Model <!--ignore--> as its own type and implement the ignoring via an assertion #220

Conversation

papandreou
Copy link
Member

@papandreou papandreou commented Sep 30, 2018

Fixes <!--ignore--> as the top-level to satisfy RHS.

… assertion

Fixes <!--ignore--> as the top-level to satisfy RHS.
@coveralls
Copy link

coveralls commented Sep 30, 2018

Pull Request Test Coverage Report for Build 820

  • 7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 91.499%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/index.js 7 8 87.5%
Totals Coverage Status
Change from base Build 814: -0.1%
Covered Lines: 525
Relevant Lines: 555

💛 - Coveralls

@alexjeffburke
Copy link
Member

This is an incredibly elegant solution given the mechanisms available in Unexpected. The modelling of the types in these assertions are such a glove for the DOM that I get this in concept but only just in details.

@sunesimonsen
Copy link
Member

@papandreou this is brilliant and how I should have made it in the first place - thanks 👍

@sunesimonsen
Copy link
Member

I'll merge this as @alexjeffburke okayed it.

@sunesimonsen sunesimonsen merged commit 006c7ec into fix/comment-nodes-in-children-satisfy-spec Oct 2, 2018
@sunesimonsen sunesimonsen deleted the fix/top-level-ignore branch October 2, 2018 17:44
@papandreou
Copy link
Member Author

Great! I wonder if the same idea can be used for <ignore>

@sunesimonsen
Copy link
Member

Yes that would make sense.

@sunesimonsen
Copy link
Member

You can probably just add the new type to all of the ignore assertions you added.

@sunesimonsen
Copy link
Member

sunesimonsen commented Oct 7, 2018

@papandreou

Great! I wonder if the same idea can be used for

I actually think we should remove the custom element ignore again. It is not documented and can't be used from unexpected-reaction. It doesn't have an element prefix, so it is not a real custom tag, and I prefer to just have one way. Would that be okay with you?

@alexjeffburke
Copy link
Member

@sunesimonsen I'd like to second you on this one - I think the rationale is solid and, for example, knockout now bans such tag definitions for custom elements outright to avoid problems with them which might be a useful datapoint.

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

4 participants