Skip to content

Update selectorAllParse to return an Array instead of a NodeList #1642

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 19, 2016
Merged

Update selectorAllParse to return an Array instead of a NodeList #1642

merged 1 commit into from
Jul 19, 2016

Conversation

@darkwing
Copy link
Contributor

@darkwing darkwing commented Jul 19, 2016

Description:
Fixes #1129

I suspect this one will require much more work. I've run tests and all passes but things to consider:

  • Is this a breaking change moving forward where API docs must be updated? Is it work the breaking change?
  • What other components may need updating to accommodate an array instead of a NodeList?
darkwing
darkwing reviewed Jul 19, 2016
View reviewed changes
src/core/propertyTypes.js

function selectorAllParse (value) {
if (!value) { return null; }
if (typeof value !== 'string') { return value; }
Copy link
Contributor Author

@darkwing darkwing Jul 19, 2016

Choose a reason for hiding this comment

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

Should value also be wrapped in Array.from()? What is the use case? In case a NodeList instance is passed in?
Copy link
Member

@dmarcos dmarcos Jul 19, 2016

Choose a reason for hiding this comment

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

We might want to return [] in the cases above?
Copy link
Member

@ngokevin ngokevin Jul 19, 2016

Choose a reason for hiding this comment

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

I think the first case may be for {default: ''} so we don't need to run that.

The second case is if an array or NodeList is passed in yeah.
@ngokevin
Copy link
Member

ngokevin commented Jul 19, 2016

looks good to me
@dmarcos dmarcos merged commit cce05b3 into aframevr:master Jul 19, 2016
@ngokevin
Copy link
Member

ngokevin commented Jul 19, 2016

@darkwing thanks!
@darkwing darkwing mentioned this pull request Aug 2, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs review

3 participants

@darkwing @ngokevin @dmarcos