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

Parser: Fix boolean attribute explicit string value result #7610

Merged
merged 2 commits into from Jun 28, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Jun 28, 2018

Related: #7322

This pull request seeks to fix an issue in the implementation of #7322 where an explicit string value assigned as an attribute would be wrongly interpreted as false when assigned as a boolean attribute type. Both <input disabled> and <input disabled="disabled"> should return true for a boolean attribute source type.

Testing instructions:

Ensure unit tests pass:

npm run test-unit blocks/api/parser.js

Repeat testing instructions from #7322.

@tofumatt

I dig it! A few comments/questions but approach is right on.

@@ -19,6 +19,22 @@ import { isValidBlock } from './validation';
import { getCommentDelimitedContent } from './serializer';
import { attr, prop, html, text, query, node, children } from './matchers';
/**
* Higher-order hpq matcher which enhances an attribute matcher to return true

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

Nice docs 👍

*
* @param {Function} matcher Original hpq matcher.
*
* @return {Function} Enhanced hpq matcher.``

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

Not sure what the backticks at the end are for 🤷‍♂️

This comment has been minimized.

@aduth

aduth Jun 28, 2018

Member

Not sure what the backticks at the end are for 🤷‍♂️

Fat fingered 😄

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

That was my guess 😆

*/
export const toBooleanAttributeMatcher = ( matcher ) => flow( [
matcher,
( value ) => value !== undefined,

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

I almost asked "what about a false value?" but that should never happen. Not sure if it's worth adding docs just to remind the reader about that.

@@ -132,6 +174,19 @@ describe( 'block parser', () => {
expect( value ).toBe( true );
} );
it( 'should return the matcher\'s true boolean attribute value on explicit attribute value', () => {

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

This is definitely a case where I'm like #7366 for the win 😉

@aduth

This comment has been minimized.

Member

aduth commented Jun 28, 2018

Thanks for the review @tofumatt . I pushed a few documentation updates in de1c2ea per your feedback.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 28, 2018

Nice, I love the table 👍

@aduth aduth merged commit 9eb1e8f into master Jun 28, 2018

2 checks passed

codecov/project 47.1% (+0.01%) compared to 3fd1328
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the fix/parser-boolean-attribute-explicit branch Jun 28, 2018

@danielbachhuber danielbachhuber added this to the 3.2 milestone Jun 28, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 28, 2018

Thanks @aduth

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