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

Add ability for various and user-defined selector patterns #13

Merged
merged 5 commits into from
Mar 13, 2015

Conversation

davidtheclark
Copy link
Contributor

An attempt at addressing #2 without dramatically altering the structure of the plugin.

I'm imagining that classic BEM or other pre-defined patterns could be added to selector-patterns.js and referenced by name; or else users could pass their own object of selector-checking patterns.

It seemed to me that this definition has to be an object instead of just a single regex to allow for (a) users to pass functions that take the component name and return a regex tailored to that component, and (b) possible variations in what can be chained to a valid ComponentName. (It looked like the existing code allowed for any selectors to follow ComponentName except classes when in strict mode. I sort of inverted that and said that the definition must provide a regexp defining precisely which chained selectors will be accepted (defaulting, for the SUIT pattern, to generic classnames, without underscores), or else any will be.)

Instead of adding tests, a classic BEM definition, etc. --- all the stuff to actually complete this functionality --- I decided to submit this initial pull request first. The code here just sets things up. That way you can review it and see what you think of the approach. Once the approach is worked out, details can be fleshed out.

I also tried to explain myself with comments in is-valid-selector.js.

@davidtheclark
Copy link
Contributor Author

@necolas Any thoughts on this? I'm wondering because if you are not planning to incorporate this kind of flexibility (in one way or another), I need to write a different module to address my particular situation, somewhat soon.

@necolas
Copy link
Contributor

necolas commented Feb 28, 2015

Sorry for the delay. I'm happy with any patches that provide that functionality; I would merge in a patch with updated docs and tests. Thank you!

@davidtheclark
Copy link
Contributor Author

Ok, cool. I'm working to improve on the prior commits and add some docs and tests. Hopefully will have something to show/review soonish.

@davidtheclark
Copy link
Contributor Author

The commit I just made does a lot. In addition to building in the functionality to allow for custom selectors:

  • I added a BEM preset pattern (as a proof of concept but also because of Support default BEM syntax #2).
  • I wrote a slew of new tests. The tests that were already in there remain as tests against the (default) SUIT pattern.
  • I wrote some documentation in the README trying to describe how a user can define custom patterns.

Please let me know what you think, @necolas. Happy to revise.

(Also, if JSCS isn't going to be workable should we switch to ESLint so Travis builds can pass?)

@necolas
Copy link
Contributor

necolas commented Mar 9, 2015

If you bump jscs to 1.11.x the errors should go away.

Might also be worth breaking the test file up into smaller test files.

I'm also happy to do other things like: revisit the 'use strict' thing (should this be the default?); names used for things (e.g., component -> element, descendant -> shadowElement); default naming pattern (or maybe you should always set the pattern you are using).

@davidtheclark
Copy link
Contributor Author

I made #15, updating dependencies, and it succeeded on Travis. So that's good.

I will submit another commit tonight or tomorrow to break up tests into smaller files.

As for the other things you mention --- I do think strict mode should be default, if you're considering that. I'm indifferent as to whether SUIT is the default pattern, don't know of a reason why it shouldn't be (I'm going to have plug in a custom pattern anyway for my own purposes).

@MoOx
Copy link

MoOx commented Mar 10, 2015

For the "use strict" thing, it is useless since module.exports implies that you are in a module environement (so strict env, ref eslint/eslint#1578). For example I think now eslint detect the auto "use strict" now - if you add the key ecmaFeatures.modules true)

@davidtheclark
Copy link
Contributor Author

@MoOx I think you are talking about JavaScript's "use strict" --- but I believe the question about strict mode here is addressing https://github.com/necolas/postcss-bem-linter#conformance-tests --- the strictness of the BEM tests ("use strict" being the statement because it is familiar to devs).

@MoOx
Copy link

MoOx commented Mar 10, 2015

Haha. Sorry. What an idiot. My bad :)

@davidtheclark
Copy link
Contributor Author

This latest commit splits up test files (and fixes some linting errors I noticed now that I got JSCS going).

@necolas
Copy link
Contributor

necolas commented Mar 13, 2015

Thanks! Please could you also add pseudo-classes to some of the test css. master produces errors for css like:

.Component:not(.is-open) { ... }

Feel free to do in a follow up. I'll merge this now.

necolas added a commit that referenced this pull request Mar 13, 2015
Add ability for various and user-defined selector patterns
@necolas necolas merged commit 44ae016 into postcss:master Mar 13, 2015
This was referenced Mar 13, 2015
@davidtheclark
Copy link
Contributor Author

Yep, I'll work on that soon.

This was referenced Mar 14, 2015
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

Successfully merging this pull request may close these issues.

None yet

3 participants