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 named-grid-areas-no-invalid #5167

Merged
merged 46 commits into from Mar 5, 2021

Conversation

evrom
Copy link
Contributor

@evrom evrom commented Feb 23, 2021

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

Closes #5155

Is there anything in the PR that needs further explanation?

Adds rule to validate CSS grid areas to the spec defined here: https://drafts.csswg.org/css-grid/#grid-template-areas-property

Node version 10, supported by the test matrix, does not support
.matchAll
because node version 10 does not support `Array.prototype.flat`
Copy link
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.

@evrom Thank you for contributing! The pull request is looking good.

I've requested a few changes. We'll want to make use of the value parser. This is probably the biggest revision. I've also requested a few changes to align the rule with our conventions so its consistent with other rules.

Once these are in place, it'll be easier to review the other workings of the rule.

(As this rule is candidate for the recommended config, we'll want to be comphensive in our tests as it's likely this rule will be run against a lot of code bases.)

lib/rules/named-grid-areas-no-invalid/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/README.md Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/README.md Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/README.md Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/README.md Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/index.js Outdated Show resolved Hide resolved
@jeddy3 jeddy3 changed the title Named grid areas no invalid Add named-grid-areas-no-invalid Feb 23, 2021
sort the non-contiguous area names alphabetically to ensure
consistency in error messages
Copy link
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.

@evrom Thank you for making all those changes! Things are looking good and I think the rule is taking shape.

I've requested some more changes.

We should focus the rule on just this part of the spec:

All strings must define the same number of cell tokens (named cell tokens and/or null cell tokens), and at least one cell token, or else the declaration is invalid. If a named grid area spans multiple grid cells, but those cells do not form a single filled-in rectangle, the declaration is invalid.

i.e.:

  • All strings must define at least one cell token.
  • All strings must define the same number of cell tokens.
  • All named grid areas that span multiple grid cells must form a single filled-in rectangle.

A few of the requested changes are around doing this.

Does that sound good to you? It may deviate from your original validator, though.

lib/rules/named-grid-areas-no-invalid/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/index.js Outdated Show resolved Hide resolved
lib/rules/named-grid-areas-no-invalid/index.js Outdated Show resolved Hide resolved
This was referenced Mar 13, 2021
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.

Add named-grid-areas-no-invalid
3 participants