Skip to content

Add declaration-property-max-values#5920

Merged
jeddy3 merged 13 commits intomainfrom
declaration-property-max-values
Mar 7, 2022
Merged

Add declaration-property-max-values#5920
jeddy3 merged 13 commits intomainfrom
declaration-property-max-values

Conversation

@mattxwang
Copy link
Copy Markdown
Member

@mattxwang mattxwang commented Feb 18, 2022 •
edited
Loading

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:

  1. Is the documentation the correct level of conciseness?
  2. In this PR, I haven't yet used the validateOptions utility function. How should I use it here, if at all?
  3. 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
ybiquitous requested changes Feb 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

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

Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/README.md
Comment thread lib/rules/declaration-property-max-values/index.js
Comment thread lib/rules/declaration-property-max-values/index.js Outdated
Comment thread lib/rules/declaration-property-max-values/index.js Outdated
Comment thread lib/rules/declaration-property-max-values/index.js Outdated
jeddy3
jeddy3 requested changes Feb 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@mattxwang Looking good so far!

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

Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/__tests__/index.js
Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/README.md Outdated
@jeddy3 jeddy3 mentioned this pull request Feb 21, 2022
Merged
mattxwang and others added 5 commits February 21, 2022 15:59
@mattxwang @ybiquitous
Apply suggestions from code review
9b6634a 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@mattxwang
slims down docs/test cases to use valid CSS, div -> a, all value nodes
f94fc00 
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
@mattxwang
creates validateObjectWithProps, validates options for declaration-pr…
62ba6c1 
…operty-max-values
@mattxwang
adds tests for validateObjectWithProps
1d2d400
@mattxwang
adds additional test case for invalid input to validateObjectWithProps
7955ad1
@mattxwang mattxwang force-pushed the declaration-property-max-values branch from 4701a2f to 7955ad1 Compare February 22, 2022 03:54
@mattxwang
Copy link
Copy Markdown
Member Author

mattxwang commented Feb 22, 2022 •
edited
Loading

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
Copy link
Copy Markdown
Member

ybiquitous commented Feb 22, 2022

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

ybiquitous
ybiquitous reviewed Feb 22, 2022
View reviewed changes
Comment thread lib/utils/__tests__/validateObjectWithProps.test.js Outdated
Comment thread lib/utils/validateObjectWithProps.js Outdated
Comment thread lib/utils/validateObjectWithProps.js Outdated
Comment thread lib/utils/validateObjectWithProps.js Outdated
Comment thread lib/utils/validateObjectWithProps.js Outdated
Comment thread lib/utils/validateObjectWithProps.js Outdated
jeddy3
jeddy3 requested changes Feb 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeddy3 jeddy3 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 making the changes! Looking good.

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

@mattxwang @ybiquitous
Apply suggestions from code review - typo fixes and validator typing
03ddbf8 
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@mattxwang
Copy link
Copy Markdown
Member Author

mattxwang commented Feb 22, 2022

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
ybiquitous reviewed Feb 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

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

I've left more trivial suggestions and one question.

Comment thread lib/rules/declaration-property-max-values/README.md Outdated
Comment thread lib/rules/declaration-property-max-values/__tests__/index.js
Comment thread lib/rules/declaration-property-max-values/index.js Outdated
Comment thread lib/rules/declaration-property-max-values/index.js Outdated
Comment thread lib/rules/declaration-property-max-values/index.js
@mattxwang @ybiquitous
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
ybiquitous approved these changes Feb 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

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

jeddy3
jeddy3 requested changes Feb 23, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@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.

Comment thread lib/rules/declaration-property-max-values/index.js Outdated
mattxwang and others added 2 commits February 23, 2022 14:39
@mattxwang @jeddy3
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>
@mattxwang
swaps border, margin configuration properties to re-add margin-inline
1d0efd9
@mattxwang
Copy link
Copy Markdown
Member Author

mattxwang commented Mar 4, 2022

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

jeddy3
jeddy3 approved these changes Mar 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeddy3 jeddy3 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 making the changes. I think the tests and readme work better now.

LGTM.

@jeddy3 jeddy3 merged commit e4917da into main Mar 7, 2022
@jeddy3 jeddy3 deleted the declaration-property-max-values branch March 7, 2022 10:09
@jeddy3
Copy link
Copy Markdown
Member

jeddy3 commented Mar 7, 2022

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

@sync-by-unito sync-by-unito Bot mentioned this pull request May 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ybiquitous ybiquitous ybiquitous approved these changes

@jeddy3 jeddy3 jeddy3 approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add declaration-property-max-values

3 participants

@mattxwang @ybiquitous @jeddy3