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

Issue #10645 - Allow BEM class via attribute tag. Public #10655

Merged
merged 2 commits into from
Aug 28, 2017
Merged

Issue #10645 - Allow BEM class via attribute tag. Public #10655

merged 2 commits into from
Aug 28, 2017

Conversation

daniloargentiero
Copy link

@daniloargentiero daniloargentiero commented Aug 25, 2017

Description

Before the fix, classes with -- in them was replaced with a single - so result with input class--1 appears as class-1, breaking the BEM convention.

Fixed Issues

  1. Adding BEM class in XML via attribute tag causes class to be rewritten #10645: Adding BEM class in XML via attribute tag causes class to be rewritten

Manual testing scenarios

  1. Go to default.xml
  2. Add attribute tag with a class to the body element i.e. <attribute name="class" value="class--1" />
  3. Clear cache and reload page

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 25, 2017

CLA assistant check
All committers have signed the CLA.

@miguelbalparda
Copy link
Contributor

miguelbalparda commented Aug 25, 2017

How about class__1? That seems to be valid for BEM classes too from here.

@miguelbalparda miguelbalparda self-assigned this Aug 25, 2017
@miguelbalparda miguelbalparda added this to the August 2017 milestone Aug 25, 2017
@@ -463,7 +463,7 @@ public function addRss($title, $href)
*/
public function addBodyClass($className)
{
$className = preg_replace('#[^a-z0-9]+#', '-', strtolower($className));
$className = preg_replace('#[^a-z0-9-]+#', '-', strtolower($className));
Copy link
Contributor

@miguelbalparda miguelbalparda Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add _ to the regex after - for classes like class__1?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Miguel,
done!

@miguelbalparda
Copy link
Contributor

This caused some internal tests to fail, working on getting those fixed.

@orlangur
Copy link
Contributor

@miguelbalparda I have some doubts on what happened with testing of this class.

Change in Config required changes in BodyTest and StructureTest "unit" tests and not in some ConfigTest.

Also, would be nice to clarify the BEM intention in test here by some testAddBodyClassSupportsBemClasses and data like 'block__elem block--hidden'.

@Ctucker9233
Copy link

@magento-team Will this be backported to 2.2?

@orlangur
Copy link
Contributor

@Ctucker9233 just backport whatever you need like @amol2jcommerce just did instead of asking 😉

@Ctucker9233
Copy link

@orlangur Just thought I would ask instead of assuming. Glad someone took care of it.

@orlangur
Copy link
Contributor

@Ctucker9233 I would say for any PR older than 3 months/1-2 minor releases contribution is unlikely to be backported by somebody from Magento.

@Ctucker9233
Copy link

@orlangur Good to know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants