magento-coding-standard icon indicating copy to clipboard operation
magento-coding-standard copied to clipboard

Less files : Class name separator issue

Open Nuranto opened this issue 2 years ago • 3 comments

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;
            }
        }
    }
}
  1. 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 :

  • remove that rule since it is not respected by M2 core code.
  • Fix class names in core M2 repo, but it will be a breaking change for all existing modules
  • add exceptions to that rule

Nuranto avatar Jun 29 '22 07:06 Nuranto

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


m2-assistant[bot] avatar Jun 29 '22 07:06 m2-assistant[bot]

@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 avatar Aug 24 '22 18:08 hostep

@svera: what's your opinion on this?

hostep avatar Aug 31 '22 20:08 hostep

@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

hostep avatar Oct 05 '22 10:10 hostep

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

bl4de avatar Oct 05 '22 17:10 bl4de

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

hostep avatar Oct 17 '22 18:10 hostep

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

bl4de avatar Oct 18 '22 08:10 bl4de

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 avatar Nov 02 '22 11:11 hostep

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

hostep avatar Nov 17 '22 16:11 hostep

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

hostep avatar Jan 17 '23 05:01 hostep