Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Less files : Class name separator issue #409

Closed
Nuranto opened this issue Jun 29, 2022 · 10 comments · Fixed by #432
Closed

Less files : Class name separator issue #409

Nuranto opened this issue Jun 29, 2022 · 10 comments · Fixed by #432
Labels
bug Something isn't working Progress: done

Comments

@Nuranto
Copy link

Nuranto commented Jun 29, 2022

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 Nuranto added the bug Something isn't working label Jun 29, 2022
@m2-assistant
Copy link

m2-assistant bot commented Jun 29, 2022

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
Copy link
Contributor

hostep commented Aug 24, 2022

@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
Copy link
Contributor

hostep commented Aug 31, 2022

@svera: what's your opinion on this?

@hostep
Copy link
Contributor

hostep commented Oct 5, 2022

@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
Copy link
Contributor

bl4de commented Oct 5, 2022

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
Copy link
Contributor

hostep commented Oct 17, 2022

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

@bl4de
Copy link
Contributor

bl4de commented Oct 18, 2022

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
Copy link
Contributor

hostep commented Nov 2, 2022

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
Copy link
Contributor

hostep commented Nov 17, 2022

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

1 similar comment
@hostep
Copy link
Contributor

hostep commented Jan 17, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Progress: done
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants