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 ignoreFontFamilyName option to font-family-no-duplicate #2314

Merged
merged 7 commits into from Feb 11, 2017

Conversation

6 participants
@CAYdenberg
Contributor

CAYdenberg commented Feb 1, 2017

Which issue, if any, is this issue related to?

Issue #2086

Is there anything in the PR that needs further explanation?

I think it's pretty self-explanatory but this is my first PR to a major project so let me know if anything needs changing.

@m-allanson

Thanks @CAYdenberg! This looks good to me, I've requested a couple of small changes to the docs.

## Optional secondary options
### `ignoreFontFamilyNames: ["monospace"]`

This comment has been minimized.

@m-allanson

m-allanson Feb 2, 2017

Member

Can you change this to show the types of values that can be accepted here?

### `ignoreFontFamilyNames: ["/regex/", "string"]`

This comment has been minimized.

@CAYdenberg

CAYdenberg Feb 5, 2017

Contributor

Addressed in ee5279e

Given:
```js
["monospace"]

This comment has been minimized.

@m-allanson

m-allanson Feb 2, 2017

Member

I think it'd be good to include a regex example here too.

This comment has been minimized.

@CAYdenberg

CAYdenberg Feb 5, 2017

Contributor

Addressed in ee5279e

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Feb 2, 2017

Good suggestions. You got me all confused about case-sensitivity so I added another test as well.

@m-allanson

This comment has been minimized.

Member

m-allanson commented Feb 6, 2017

@CAYdenberg 👍 for extra tests.

I should have explained myself more clearly for the docs changes in the previous comments.

The ignoreFontFamilyNames title should show the types of values it accepts - regexes or strings e.g. ["/regex/", "string"]

Then the example usage should show an example regex and an example string. e.g. ["/^my-/", "custom"]

You can use the ignoreAtRules option of at-rule-no-unknown as a reference.

Can you update those parts of the readme along with the regex code example on L66? This might all seem overly fussy but there are a lot of rules in stylelint so we aim for strong consistency across the docs.

Other than the docs update this LGTM, thanks again for working on it.

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Feb 7, 2017

+1 for fussiness - I don't think someone who isn't fussy would be using/contributing to/writing a CSS linter.

Is the issue that the example ("/Regex/") doesn't look enough like a regular expression? I mean, it could be, but I guess I see your point. Most font family names are capitalised and have spaces (not hyphens) so I guess my thought was there would be some value in providing an example that looks like something someone might actually use. Would ("/^Regex/") be more appropriate? Or would you prefer `("/^my-/") since it's closer to the other examples?

I do see what you mean about the title - I just outright missed that.

@m-allanson

This comment has been minimized.

Member

m-allanson commented Feb 7, 2017

Is the issue that the example ("/Regex/") doesn't look enough like a regular expression?

Exactly - I think it's confusing if the example regex includes the string "Regex" in it. I see your point about font family names though. We could use a more realistic example regex - "/^My Font /" or "/^CustomFamily/"?

So we'd have ["/regex/", "string"] in the title, and something like ["/^My Font /", "custom"] in the example.

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Feb 8, 2017

Got it. That should do it? I left monospace as the example string since that will probably cover most use cases for this option. Other than that it's exactly as you prescribed.

@m-allanson

This comment has been minimized.

Member

m-allanson commented Feb 8, 2017

That LGTM, thanks again!

@CAYdenberg CAYdenberg changed the title from Issue 2086 to Add ignoreFontFamilyName option to font-family-no-duplicate Feb 9, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 11, 2017

Looks great. Thanks @CAYdenberg! I notice you're doing a lot to help out!

@davidtheclark davidtheclark merged commit 74d9d05 into stylelint:master Feb 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 11, 2017

Added to changelog:

  • Added: ignoreFontFamilyName option to font-family-no-duplicate (#2314).

@CAYdenberg CAYdenberg deleted the CAYdenberg:issue_2086 branch Feb 13, 2017

m-allanson added a commit that referenced this pull request Feb 22, 2017

Merge branch 'master' into jest-coverage
* master: (107 commits)
  Update jest to the latest version 🚀 (#2372)
  fix(package): update known-css-properties to version 0.0.7
  chore(package): update eslint to version 3.16.0
  Update CHANGELOG.md
  Add support for async rules (#2351)
  Prepare 7.9.0
  Add a test for a parent selector in no-duplicate-selectors (#2364)
  Update CHANGELOG.md
  Add ignorePattern option to max-line-length (#2333)
  no-browser-hacks: link to external plugin (#2365)
  Update CHANGELOG.md
  Fix placeholder selector case (#2360)
  Always exclude ignored globs from globby search. (#2355)
  Update CHANGELOG.md
  Update lodash (#2353)
  Update CHANGELOG.md
  Add ignoreFontFamilyName option to font-family-no-duplicate (#2314)
  Tests: added tests for nested selectors in `selector-id-pattern` rule. (#2345)
  Fix brace position (#2347)
  Update CHANGELOG.md
  ...

# Conflicts:
#	.gitignore

hudochenkov added a commit that referenced this pull request Feb 23, 2017

Merge branch 'master' into v8
* master: (31 commits)
  Update stylehacks and snapshot
  Update jest to the latest version 🚀 (#2372)
  fix(package): update known-css-properties to version 0.0.7
  chore(package): update eslint to version 3.16.0
  Update CHANGELOG.md
  Add support for async rules (#2351)
  Prepare 7.9.0
  Add a test for a parent selector in no-duplicate-selectors (#2364)
  Update CHANGELOG.md
  Add ignorePattern option to max-line-length (#2333)
  no-browser-hacks: link to external plugin (#2365)
  Update CHANGELOG.md
  Fix placeholder selector case (#2360)
  Always exclude ignored globs from globby search. (#2355)
  Update CHANGELOG.md
  Update lodash (#2353)
  Update CHANGELOG.md
  Add ignoreFontFamilyName option to font-family-no-duplicate (#2314)
  Tests: added tests for nested selectors in `selector-id-pattern` rule. (#2345)
  Fix brace position (#2347)
  ...

# Conflicts:
#	CHANGELOG.md

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

Add ignoreFontFamilyName option to font-family-no-duplicate (stylelin…
…t#2314)

* Adds tests for ignoreFontFamilyName option to font-family-no-duplicate-names

* Add secondary option ignoreFontFamilyNames to font-family-no-duplicate-names

* Add documentation for secondary option ignoreFontFamilyNames

* Fix missing final newline in MD file

* Add documentation describing regex

* Add an additional test for case sensitivity in regex ignoreFontFamilyNames rule

* Improve explanation of ignoreFontFamilyNames
@webdesignberlin

This comment has been minimized.

webdesignberlin commented Aug 9, 2017

Any Example there, how i can use the ignoreFontFamilyName Rule within the stylelintrc?

I have try this:

"font-family-no-duplicate-names": [ true, { "ignore-font-family-name": ["monospace"] } ],

But i became an error:
Invalid option name "ignore-font-family-name" for rule "font-family-no-duplicate-names"

Version:
$ ./node_modules/.bin/stylelint -v 8.0.0

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Aug 9, 2017

@webdesignberlin use ignoreFontFamilyNames instead ignore-font-family-name

@webdesignberlin

This comment has been minimized.

webdesignberlin commented Aug 9, 2017

Oje, he, she, it the s goes with :) ignoreFontFamilyNames works.

via Source Code:

if (optionsMatches(options, "ignoreFontFamilyNames", fontFamilyNode.value.trim())) {

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Aug 21, 2017

For future visitors... the ignoreFontFamilyNames option is documented here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment