magento / magento/magento-coding-standard

Allow multiline css declarations in less files for increased readability

Open
#471 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug Progress: ready for grooming
Dominant language
PHP
Stars
375
Forks
165
PR merge metrics
No merged PRs in 30d

Description

### Preconditions
1. Have a file `test1.less` like this:
```less
& when (@media-common = true) {
.lib-icon-font(
@icon-camera__content,
@_icon-font: @icons-admin__font-name,
@_icon-font-size: @image-gallery-placeholder-icon__size,
@_icon-font-color: @image-gallery-placeholder-icon__color,
@_icon-font-text-hide: true
);
}
```
2. Have a file `test2.less` like this:
```less
& when (@media-common = true) {
.lib-icon-font(@icon-camera__content, @_icon-font: @icons-admin__font-name, @_icon-font-size: @image-gallery-placeholder-icon__size, @_icon-font-color: @image-gallery-placeholder-icon__color, @_icon-font-text-hide: true);
}
```
3. See that the contents of these 2 files are the same, it's just the formatting of the code that's different

### Steps to reproduce
1. Run `vendor/bin/phpcs -s --standard=Magento2 test*.less`

### Expected result
1. Both files should show no errors

### Actual result
1. Getting this output:
```
FILE: test1.less
-------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
-------------------------------------------------------------------------------------------------------------------------
4 | WARNING | Expected 1 space after colon in style definition; newline found
| | (Magento2.Less.ColonSpacing.AfterNewline)
5 | WARNING | Expected 1 space after colon in style definition; newline found
| | (Magento2.Less.ColonSpacing.AfterNewline)
6 | WARNING | Expected 1 space after colon in style definition; newline found
| | (Magento2.Less.ColonSpacing.AfterNewline)
7 | WARNING | Expected 1 space after colon in style definition; newline found
| | (Magento2.Less.ColonSpacing.AfterNewline)
-------------------------------------------------------------------------------------------------------------------------

Time: 78ms; Memory: 6MB
```

### Discussion
In my opinion, the formatting of the code in file `test1.less` is a lot more readable then in file `test2.less`. Coding standards shouldn't recommend to make code less readable.
Also, Magento [uses this syntax as well in its own code](https://github.com/magento/magento2/blob/8ea62ab9374a9e14a9ffe615aceedff745d80077/app/design/adminhtml/Magento/backend/web/css/source/components/_file-uploader.less#L274-L280) and is thus violating its own coding standards ...

This was already reported before, but just [in a comment](https://github.com/magento/magento-coding-standard/issues/395#issuecomment-1503527962) on another issue, now it has its own proper issue which might make it faster to get picked up?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Run vendor/bin/phpcs -s --standard=Magento2 against the two test*.less examples and inspect the Magento2.Less.ColonSpacing.AfterNewline rule. Add regression coverage for multiline Less declarations, using the existing Magento2 standard tests or the rule's test entry point if present. Done means both equivalent files produce no warnings.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.