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

spaces on selectors (or containers in general) don't work #172

Open
jquense opened this issue Feb 19, 2019 · 9 comments
Open

spaces on selectors (or containers in general) don't work #172

jquense opened this issue Feb 19, 2019 · 9 comments

Comments

@jquense
Copy link

jquense commented Feb 19, 2019

parser
  .pseudo({
    value: ':not',
    nodes: [
      selectorParser.selector({
        nodes: [selectorParser.tag({ value: 'span' })],
        spaces: { before: ' ', after: ' ' },
      }),
      selectorParser.selector({
        nodes: [selectorParser.tag({ value: 'span' })],
        spaces: { before: ' ', after: ' ' },
      }),
    ],
  })
  .toString() // ':not(span,span)'

I think the idea is that the spaces go on the nodes, but that's a bit unwieldy of an API when walking through the or transforming selectors, since you drill down and find the first non-container thing to apply spacing too.

@alexander-akait
Copy link
Collaborator

@jquense thanks for issue, why don't use rawSpaceBefore and rawSpaceAfter?

@jquense
Copy link
Author

jquense commented Feb 19, 2019

i don't know how too! :) I didn't see that in the documentation. The problem I'm running into here is the case where we replace a pseudo selector with it's children.

its a bit awkward to do: psuedo.first.first.spaces = psuedo.spaces

@alexander-akait
Copy link
Collaborator

@jquense hm, rawSpaceBefore and rawSpaceAfter don't work or it is inconvenient?

@jquense
Copy link
Author

jquense commented Feb 19, 2019

they may work, i just don't know what they are. looking at the code tho, toString() for containers doesn't use them?

@alexander-akait
Copy link
Collaborator

@jquense here contains https://github.com/postcss/postcss-selector-parser/blob/master/src/selectors/node.js#L180

@jquense
Copy link
Author

jquense commented Feb 19, 2019

gotcha, yeah that works for a node but selectors are containers and they don't call super() on toString() https://github.com/postcss/postcss-selector-parser/blob/master/src/selectors/container.js#L317

This may be intentional, it's hard to know if you what to do if you put spaces on both the first node and the selector itself. Overall its more inconvenient than anything, and makes the case where the inner node doesn't exist yet hard

@alexander-akait
Copy link
Collaborator

alexander-akait commented Feb 19, 2019

@jquense do you have ideas how we can improve this? You can send a PR and we can discussion in a PR

@alexander-akait
Copy link
Collaborator

/cc @jquense friendly ping

@jquense
Copy link
Author

jquense commented Feb 26, 2019

I don't have a good suggestion right now unfortunately. I'd be nicer if the logic in postcss-local-by-default could be simplified but i'm not sure the right way to handle it given the way this package works

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

No branches or pull requests

2 participants