Add declaration-property-max-values #5920

mattxwang · 2022-02-18T23:11:16Z

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

Closes #5435, related to #3968.

Is there anything in the PR that needs further explanation?

As suggested by @jeddy3, I've scaffolded this off of declaration-property-value-allowed-list. I've also implemented a new util, validateObjectWithProps(), and written some tests.

This is my first time creating an entirely new rule, so I have three things I'd like to get some feedback on:

Is the documentation the correct level of conciseness?
In this PR, I haven't yet used the validateOptions utility function. How should I use it here, if at all?
My method for finding the number of values is finding the number of word nodes after running the value through the value parser. Is this the correct approach?

ybiquitous

@mattxwang Thank you for creating this pull request! I think the implementation is in the right direction. 👍🏼

lib/rules/declaration-property-max-values/README.md

lib/rules/declaration-property-max-values/index.js

jeddy3

@mattxwang Looking good so far!

I've requested a few changes in addition to @ybiquitous'.

lib/rules/declaration-property-max-values/README.md

lib/rules/declaration-property-max-values/__tests__/index.js

lib/rules/declaration-property-max-values/README.md

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

Applies suggestions from code review: - removes invalid CSS from docs and test cases - removes redundant docs entries - uses `a` for all selectors - writes a new helper "isValueNode" to cover word, function, string value types - adds new test case for css variable

…operty-max-values

mattxwang · 2022-02-22T04:01:40Z

Thanks for the in-depth feedback @jeddy3 and @ybiquitous, it's much appreciated - I still have a lot to learn in terms of the codebase and best practices.

In general, I think I've addressed each point of feedback. I did have a chance to implement validateObjectWithProps, though I'm not sure if it's the intended behaviour from the above discussion - is this what you two were thinking? Did you want me to add the array functionality from validateObjectWithArrayProps as well?

And, anything else I should be mindful of?

ybiquitous · 2022-02-22T07:13:32Z

@mattxwang I think your implementation of validateObjectWithProps is just what we want. 👍🏼

lib/utils/__tests__/validateObjectWithProps.test.js

lib/utils/validateObjectWithProps.js

jeddy3

Thanks for making the changes! Looking good.

In addition to @ybiquitous' suggestion, just one more minor request from me

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

mattxwang · 2022-02-22T21:55:48Z

Thanks for the suggestions @ybiquitous, didn't realize that we'd get better typing by importing from ./validateTypes!

@jeddy3, I may be missing something, but what was your requested change? I don't see anything attached to your "requested changes" dialog box.

ybiquitous

@mattxwang Thank you for addressing the reviews. Almost good. 👍🏼

I've left more trivial suggestions and one question.

lib/rules/declaration-property-max-values/README.md

lib/rules/declaration-property-max-values/__tests__/index.js

lib/rules/declaration-property-max-values/index.js

Moves `validateOptions` outside of `walkDecls`, better type annotation style! Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

ybiquitous

@mattxwang Thank you for implementing the excellent rule. LGTM 👍🏼

jeddy3

@jeddy3, I may be missing something, but what was your requested change? I don't see anything attached to your "requested changes" dialog box.

That's weird. It seems it fell into the ether.

I've readded it.

lib/rules/declaration-property-max-values/index.js

Moves external require to top of require list Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>

mattxwang · 2022-03-04T02:01:18Z

Completely missed one of the requested changes, my fault! Should have addressed all the comments - @jeddy3 let me know what you think!

jeddy3

Thanks for making the changes. I think the tests and readme work better now.

LGTM.

jeddy3 · 2022-03-07T10:10:15Z

Added: declaration-property-max-values rule (#5920).

mattxwang added 2 commits February 18, 2022 15:10

drafts declaration-property-max-values

206da50

adds extra test case for irrelevant property, removes redundant code

bf017ea

ybiquitous requested changes Feb 20, 2022

View reviewed changes

jeddy3 requested changes Feb 20, 2022

View reviewed changes

jeddy3 added the pr: needs revision label Feb 20, 2022

jeddy3 mentioned this pull request Feb 21, 2022

Document test and README example conventions #5928

Merged

mattxwang and others added 5 commits February 21, 2022 15:59

Apply suggestions from code review

9b6634a

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

creates validateObjectWithProps, validates options for declaration-pr…

62ba6c1

…operty-max-values

adds tests for validateObjectWithProps

1d2d400

adds additional test case for invalid input to validateObjectWithProps

7955ad1

mattxwang force-pushed the declaration-property-max-values branch from 4701a2f to 7955ad1 Compare February 22, 2022 03:54

Merge branch 'main' into declaration-property-max-values

2430b2a

ybiquitous reviewed Feb 22, 2022

View reviewed changes

jeddy3 requested changes Feb 22, 2022

View reviewed changes

Apply suggestions from code review - typo fixes and validator typing

03ddbf8

Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

removes unused template type

57c4f4e

ybiquitous reviewed Feb 23, 2022

View reviewed changes

Apply suggestions from code review

c5889cd

Moves `validateOptions` outside of `walkDecls`, better type annotation style! Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>

ybiquitous approved these changes Feb 23, 2022

View reviewed changes

jeddy3 requested changes Feb 23, 2022

View reviewed changes

lib/rules/declaration-property-max-values/index.js Outdated Show resolved Hide resolved

mattxwang and others added 2 commits February 23, 2022 14:39

Update lib/rules/declaration-property-max-values/index.js

6a5d57b

Moves external require to top of require list Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>

swaps border, margin configuration properties to re-add margin-inline

1d0efd9

jeddy3 removed the pr: needs revision label Mar 7, 2022

jeddy3 approved these changes Mar 7, 2022

View reviewed changes

jeddy3 merged commit e4917da into main Mar 7, 2022

jeddy3 deleted the declaration-property-max-values branch March 7, 2022 10:09

sync-by-unito bot mentioned this pull request May 1, 2022

build(deps-dev): bump stylelint and stylelint-config-standard numbersprotocol/capture-lite#1570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add declaration-property-max-values #5920

Add declaration-property-max-values #5920

mattxwang commented Feb 18, 2022 •

edited

ybiquitous left a comment

jeddy3 left a comment

mattxwang commented Feb 22, 2022 •

edited

ybiquitous commented Feb 22, 2022

jeddy3 left a comment

mattxwang commented Feb 22, 2022

ybiquitous left a comment

ybiquitous left a comment

jeddy3 left a comment

mattxwang commented Mar 4, 2022

jeddy3 left a comment

jeddy3 commented Mar 7, 2022

Add declaration-property-max-values #5920

Add declaration-property-max-values #5920

Conversation

mattxwang commented Feb 18, 2022 • edited

ybiquitous left a comment

Choose a reason for hiding this comment

jeddy3 left a comment

Choose a reason for hiding this comment

mattxwang commented Feb 22, 2022 • edited

ybiquitous commented Feb 22, 2022

jeddy3 left a comment

Choose a reason for hiding this comment

mattxwang commented Feb 22, 2022

ybiquitous left a comment

Choose a reason for hiding this comment

ybiquitous left a comment

Choose a reason for hiding this comment

jeddy3 left a comment

Choose a reason for hiding this comment

mattxwang commented Mar 4, 2022

jeddy3 left a comment

Choose a reason for hiding this comment

jeddy3 commented Mar 7, 2022

mattxwang commented Feb 18, 2022 •

edited

mattxwang commented Feb 22, 2022 •

edited