Skip to content

feat: add prefer-logical-properties rule#63

Merged
nzakas merged 8 commits into
eslint:mainfrom
azat-io:feat/prefer-logical-properties
Mar 3, 2025
Merged

feat: add prefer-logical-properties rule#63
nzakas merged 8 commits into
eslint:mainfrom
azat-io:feat/prefer-logical-properties

Conversation

@azat-io

@azat-io azat-io commented Feb 22, 2025

Copy link
Copy Markdown
Contributor

Prerequisites checklist

What is the purpose of this pull request?

Add new rule.

What changes did you make? (Give an overview)

Added new rule for usage logical CSS properties instead of phisical.

Related Issues

#13

Is there anything you'd like reviewers to focus on?

I don't think so.

@nzakas

nzakas commented Feb 24, 2025

Copy link
Copy Markdown
Member

Thanks for getting this started. There were some edge cases that were discussed in the original issue:

#13 (comment)
#13 (comment)

Can you describe how you're handling those?

@azat-io

azat-io commented Feb 24, 2025

Copy link
Copy Markdown
Contributor Author

I don't think we need to have an option to exclude certain properties. Since all properties have roughly the same support by browsers. With the exception of logical units only.

What options would you like to see in the rule?

@nzakas

nzakas commented Feb 25, 2025

Copy link
Copy Markdown
Member

I think we need a way for people to opt-out on a case-by-case basis. At a minimum, I think we'd need to be able to do this:

{
    "prefer-logical-properties": ["error", {
        allowedProperties: ["padding-left"],   // don't warn on these
        allowedUnits: ["xyz"]  // don't warn on these
    }
}

Comment thread src/rules/prefer-logical-properties.js Outdated
@azat-io

azat-io commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

I've added options allowedUnits and allowedProperties ✅.

@nzakas

nzakas commented Feb 26, 2025

Copy link
Copy Markdown
Member

Thanks, this is looking good. A couple of additional asks:

  1. Can you changed allowedProperties to allowProperties and allowedUnits to allowUnits? I just went through and checked which convention we are using in other rules and it's "allow" rather than "allowed".
  2. Can you also update the docs to mention the new options?

Comment thread src/rules/prefer-logical-properties.js Outdated
meta: {
type: /** @type {const} */ ("problem"),

fixable: "code",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To fix the type error.

Suggested change
fixable: "code",
fixable: /** @type {const} */ ("code"),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed ✅

@azat-io

azat-io commented Feb 26, 2025

Copy link
Copy Markdown
Contributor Author

Renamed the options and added their descriptions to the documentation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just to finish up the docs, we typically include two additional sections:

  1. When Not to Use It - gives guidance on when it's okay to not use the rule
  2. Prior Art - links to other tools that do the same thing

Here's an example:
https://github.com/eslint/css/blob/main/docs/rules/no-invalid-properties.md

For prior art, we have:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added ✅

@nzakas

nzakas commented Feb 26, 2025

Copy link
Copy Markdown
Member

This is looking good. I just realized that it will also error for @supports rules, such as:

@supports (text-align: left) {
    /* rules */
}

I'm not sure if that's the correct behavior. What do you think?

@fasttime fasttime added this to Triage Feb 27, 2025
@github-project-automation github-project-automation Bot moved this to Needs Triage in Triage Feb 27, 2025
@jeddy3

jeddy3 commented Feb 27, 2025

Copy link
Copy Markdown

Looking good! Two questions...

Do we want the rule to also flag the width and height size features (useful for alignment with the container type)?, i.e. allow this:

main {
  container-type: inline-size;

  & h2 {
    @container (inline-size > 40em) {
      /* .. */
    }
  }

While disallowing this:

main {
  container-type: inline-size;

  & h2 {
    @container (width > 40em) {
      /* .. */
    }
  }

And do we want to do anything special with overflow-x and overflow-y as their logical counterparts are only supported by Firefox?

@nzakas nzakas moved this from Needs Triage to Implementing in Triage Feb 27, 2025
@nzakas

nzakas commented Feb 27, 2025

Copy link
Copy Markdown
Member

And do we want to do anything special with overflow-x and overflow-y as their logical counterparts are only supported by Firefox?

I think we can skip these for now. If/when other browsers implement the logical equivalent we can revisit.

Do we want the rule to also flag the width and height size features (useful for alignment with the container type)?,

These aren't properties, so I'm not sure it makes sense to bundle this check in this rule.

@azat-io

azat-io commented Mar 1, 2025

Copy link
Copy Markdown
Contributor Author

This is looking good. I just realized that it will also error for @supports rules

You are right. We should not use linting in @supports directives. Stylelint's rules don't do that either.

I've updated the PR and added new tests.

image

@nzakas nzakas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks for all of your work on this.

(The CI failure is unrelated to this change, so we're not going to worry about that.)

@nzakas nzakas merged commit 2a440ce into eslint:main Mar 3, 2025
@github-project-automation github-project-automation Bot moved this from Implementing to Complete in Triage Mar 3, 2025
@github-actions github-actions Bot mentioned this pull request Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

4 participants