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

Token List: Add missing stringifier and iterator #11825

Merged
merged 3 commits into from Nov 19, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Nov 13, 2018

This pull request seeks to implement missing stringifier and iterator behaviors for the @wordpress/token-list module.

While it's not very well described, these two features are clearly contained in the specification of the DOMTokenList interface:

image

You can further verify this behavior in the browser by running the following command in the console after using the Right Click > Inspect Element option:

String( $0.classList )

Testing instructions:

Ensure unit tests pass:

npm run test-unit

This specific functionality is not currently used in the application, but was considered in a yet-to-be-proposed resolution for #11352.

@jorgefilipecosta

LGTM 👍
To test this PR I executed the following code in the browser console:

var token = new wp.tokenList.default( 'aaa bbbb' );
[ ...token ]; // got ["aaa", "bbbb"]; //on master an error occurred
var iterator = token[Symbol.iterator](); // on master an error occurred
iterator.next(); // got {value: "aaa", done: false}
token.ToString(); // got "aaa bbbb"// on master an error occurred

All this test cases are also covered by the automattic tests being added.

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

new wp.tokenList.default( 'aaa bbbb' );

It appears like you stumbled into another unrelated bug by accident 😄

This should not need .default. It's the purpose of @wordpress/library-export-default-webpack-plugin.

We just need to include token-list here:

gutenberg/webpack.config.js

Lines 165 to 170 in 1dfee17

new LibraryExportDefaultPlugin( [
'api-fetch',
'deprecated',
'dom-ready',
'redux-routine',
].map( camelCaseDash ) ),

I can plan to whip up a pull request.

@aduth

This comment has been minimized.

Member

aduth commented Nov 16, 2018

Additionally, I'm wondering if the global should be named wp.TokenList to set the expectation that it's constructable .

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Nov 16, 2018

It appears like you stumbled into another unrelated bug by accident 😄

This should not need .default. It's the purpose of @wordpress/library-export-default-webpack-plugin.

I found this need strange and I was going to check later if there was a special reason in this case for things to be like that, thank you for checking this 👍

Additionally, I'm wondering if the global should be named wp.TokenList to set the expectation that it's constructable.

I think we should rename it, it's not expectable that wp.tokenList is a constructor. It is not the best time to rename things, but I don't think TokenList as many external usages yet, and the advantage of making the code and its usage more clear is worth it.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 19, 2018

An alternative solution might be:

export { TokenList };

function tokenList( initialValue ) {
    return new TokenList( initialValue );
}

export default tokenList;

(to avoid the need to rename the global variable)

@aduth

This comment has been minimized.

Member

aduth commented Nov 19, 2018

@gziolo I don't know that we could support both default and named exports from a module, without also having the undesirable .default property on the global?

@aduth aduth merged commit 41a5f2e into master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the add/token-list-missing branch Nov 19, 2018

@gziolo gziolo added this to the 4.5 milestone Nov 19, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Token List: Add missing stringifier and iterator (WordPress#11825)
* Token List: Add stringifier

* Token List: Add iterator

* Token List: Add tests for array-inherited methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment