magento-coding-standard
magento-coding-standard copied to clipboard
Less files : Class name separator issue
Preconditions
M2.4.4 Coding standard v25
Steps to reproduce
- Create a module
- 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;
}
}
}
}
- 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
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
- Join Magento Community Engineering Slack and ask your questions in #github channel.
@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.
@svera: what's your opinion on this?
@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:
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
Hi @bl4de: have you guys already found some time to discuss this?
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
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 ...)
Hi @bl4de, @sivaschenko, @svera, ...: really sorry to keep bothering you guys, but is there any sort of update around this from you?
Hi @bl4de, @sivaschenko, @svera, ...: really sorry to keep bothering you guys, but is there any sort of update around this from you?