magento / magento-coding-standard

Magento Coding Standard
Open Software License 3.0
349 stars 153 forks source link

Less files : Class name separator issue #409

Closed Nuranto closed 1 year ago

Nuranto commented 2 years ago

Preconditions

M2.4.4 Coding standard v25

Steps to reproduce

  1. Create a module
  2. Add file view/css/source/_module.less with content :
    /**
    * @DoNotInject
    */
    .admin__menu {
    .item-mymodule.level-0 {
        & > a {
            min-height: auto;
            padding: 1rem;
            &:before {
                content: url('Namespace_Module::images/module-logo.png');
                display: block;
                height: 28px;
                margin-bottom: .5rem;
            }
        }
    }
    }
  3. Launch phpcs

Expected result

No errors

Actual result

4 | WARNING | CSS class names should be separated with "-" (dash)
    |         | instead of "_" (underscore)

Notes

We should either :

m2-assistant[bot] commented 2 years ago

Hi @Nuranto. Thank you for your report. To speed up processing of this issue, make sure that you provided sufficient information.

Add a comment to assign the issue: @magento I am working on this


hostep commented 2 years ago

@sivaschenko: what do you think about this suggestion? I'm all for it.

If you don't want to allow underscores, then at least allow .admin__* to be used, because Magento's codebase is full of these classes.

hostep commented 2 years ago

@svera: what's your opinion on this?

hostep commented 2 years ago

@bl4de: maybe you have an opinion?

Coding standards should make code more consistent and more readable, having to add things like these doesn't make code more readable but the opposite, it makes it harder to read:

Screenshot 2022-10-05 at 12 22 55
bl4de commented 2 years ago

Hello @hostep,

Thank you for your contribution! We will discuss this issue internally and create corresponding issue in the dev guild. I will get back to you when I will have any update regarding your PR.

Regards, Rafal

hostep commented 2 years ago

Hi @bl4de: have you guys already found some time to discuss this?

bl4de commented 2 years ago

Hi @hostep,

We have discussed this issue internally and decided to take a look at it. Currently we are reviewing possible solutions. I will keep you updated. Regards,

Rafal

hostep commented 2 years ago

Hi @bl4de: is there already some more news? (sorry to keep bugging you but I feel like it's important to get this figured out soon, so pull requests that keep running into this silly check can soon be resolved then ...)

hostep commented 2 years ago

Hi @bl4de, @sivaschenko, @svera, ...: really sorry to keep bothering you guys, but is there any sort of update around this from you?

hostep commented 1 year ago

Hi @bl4de, @sivaschenko, @svera, ...: really sorry to keep bothering you guys, but is there any sort of update around this from you?