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 ignoreFunctions: [] to length-zero-no-unit #5082

Conversation

JuanFML
Copy link

@JuanFML JuanFML commented Dec 18, 2020

Added condition to ignore when function is detected and a test to check it with.

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

Fixes issue #4945

Is there anything in the PR that needs further explanation?

I left some comments in the code as it is another type of solution, this is my first PR, so please let me know if the solution wasn't what you were looking for, in order to fix it. There would be still left to add to the README.md file the new option added, although I wanted to get it checked first before adding it.

Added conditions and tests
Copy link
Member

@vankop vankop left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, some comments:

lib/rules/length-zero-no-unit/index.js Outdated Show resolved Hide resolved
lib/rules/length-zero-no-unit/index.js Outdated Show resolved Hide resolved
@vankop
Copy link
Member

vankop commented Jan 9, 2021

@JuanFML I have reworked things a little bit, take a look

@JuanFML
Copy link
Author

JuanFML commented Jan 9, 2021

@vankop Yeah they work perfectly your changes, thanks a lot! I don't have to commit again do I? With your commit is all done?

@vankop
Copy link
Member

vankop commented Jan 9, 2021

I don't have to commit again do I?

yes, all is fine

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

Could you add test cases, please?

/* it should report only first 0px */
a { margin: 0px var(--prop, 0px) }

/* it should report only second 0px */
a { margin: calc(5px + var(--prop, 0px)) 0px }

@jeddy3 jeddy3 mentioned this pull request Jan 11, 2021
6 tasks
@mattxwang mattxwang changed the title Added option to length-zero-no-unit. Add ignoreFunctions: [] to length-zero-no-unit Jan 25, 2021
@jeddy3
Copy link
Member

jeddy3 commented Apr 24, 2021

Revisiting after the refactoring in #5256.

I think a simple conditional just after line 52 should work now:

if (isFunction(valueNode) && optionsMatches(secondary, 'ignoreFunctions', value)) return false;

With the following tests:

testRule({
	ruleName,
	config: [true, { ignoreFunctions: ['var', /^--/] }],
	accept: [
		{
			code: 'a { top: var(--foo, 0px); }',
		},
		{
			code: 'a { top: --bar(0px); }',
		},
	],
	reject: [
		{
			code: 'a { margin: var(--foo, 0px) 0px --bar(0px); }',
			fixed: 'a { margin: var(--foo, 0px) 0 --bar(0px); }',

			message: messages.rejected,
			line: 1,
			column: 30,
		},
	],
});

We'll need to require lodash on line 5:

const _ = require('lodash');

And update the options validator on line 41:

ignoreFunctions: [_.isString, _.isRegExp],

And add a isFunction function on line 154:

function isFunction({ type }) {
	return type === 'function';
}

The docs can look like this, e.g.

Given:

["var", "/^--/"]

The following patterns are not considered violations:

a { top: var(--foo, 0px); }
a { top: --bar(0px); }

@JuanFML Would you like to update the pull request?

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

Successfully merging this pull request may close these issues.

None yet

5 participants