Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Possible solution for #118 #119

Merged
merged 1 commit into from
Jul 9, 2017

Conversation

bencripps
Copy link
Contributor

This is one possible solution to the issue I posted. Not sure if this is how you'd like to resolve this, or if you have a higher level solution (that handles more than just the @extend example).

Let me know what you think; happy to make suggested changes.

return child.type === 'atrule' && child.name === 'extend'
});
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to think about more than one extend here? Like:

.selector {
  @extend .test;
  @extend .blah;
}

Perhaps hasNoDeclarations needs to be repurposed/renamed to handle extra cases so that we only need to iterate with every once.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, I think that's a very real use case. I like the separation of having two functions, but I understand your point. If we kept the functions separate it would look like:

function isOnlyExtends(node) {
  return node.nodes && node.nodes.length && node.every(function(child) { 
    return child.type === 'atrule' && child.name === 'extend'
  });
}

What do you envision the repurposed version would look like?

Copy link
Collaborator

@simonsmith simonsmith Jul 8, 2017

Choose a reason for hiding this comment

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

On second thought I think your idea is better and the minimal overhead of two iterations allows the code to be clearer which I think is more important. Seeing as we now check the node.nodes in two places perhaps we could do:

function hasChildNodes(node) {
  return node.nodes && node.nodes.length;
}

function hasNoDeclarations(node) {
  return hasChildNodes(node) && node.every(function(child) {
    return child.type !== 'decl';
  });
}

function hasOnlyExtends(node) {
  return hasChildNodes(node) && node.every(function(child) { 
    return child.type === 'atrule' && child.name === 'extend';
  });
}

I was thinking that has instead of is reads a little better too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I think this works. I'll amend my commit and push.

@bencripps bencripps force-pushed the bugfix/dont-skip-extends branch from 46bf50d to a98ca54 Compare July 8, 2017 21:12
@simonsmith
Copy link
Collaborator

Could you amend the commit message to be a bit more descriptive? Otherwise I think this looks good!

@bencripps bencripps force-pushed the bugfix/dont-skip-extends branch from a98ca54 to 56ba39f Compare July 8, 2017 23:30
@bencripps
Copy link
Contributor Author

Amended and pushed, thanks!

@simonsmith simonsmith merged commit ebd44a8 into postcss:master Jul 9, 2017
@simonsmith
Copy link
Collaborator

Great stuff, thanks @bencripps

@bencripps
Copy link
Contributor Author

Great, any chance of a getting this into a new release?

@simonsmith
Copy link
Collaborator

@bencripps Got a couple more changes to make and then I'll get a release out ASAP

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants