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

Change _getHtml to append class rather than overwrite for children #12862

Merged
merged 2 commits into from
Jan 7, 2018

Conversation

jonshipman
Copy link

@jonshipman jonshipman commented Dec 22, 2017

Change _getHtml to append class rather than overwrite for children

Description

When creating a dependency injection for the Magento\Theme\Block\Html\Topmenu class, we are unable to change class names on children in a beforeGetHtml method because the protected method _getHtml declares setClass() on all children items. What this contribution changes is checking each child for an existing class and appends the $outermostClass if true.

Manual testing scenarios

  1. Create a di.xml in a custom module that creates a plugin for the Magento\Theme\Block\Html\Topmenu class.
  2. In a beforeGetHtml injection, add a loop over the (Magento\Theme\Block\Html\Topmenu) $subject->getMenu()->getChildren() and ->setClass() on each one.
  3. With this contribution, each child gets the class specified. Without, it gets overwritten.

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 Dec 22, 2017

CLA assistant check
All committers have signed the CLA.

@dmanners
Copy link
Contributor

dmanners commented Jan 2, 2018

@jonshipman thank you for the pull request. Could you sign the CLA and then we can start to progress this pull request.

@jonshipman
Copy link
Author

I give my consent, the CLA button says I sign it but this PR doesn't seem to update. Attached is the image I see.

https://imgur.com/9mMXGXC

@dmanners
Copy link
Contributor

dmanners commented Jan 3, 2018

@jonshipman it seems that the account the commit is assigned to (https://github.com/jonshipmansmc) is different from the account you are creating the pr and signing the CLA with (https://github.com/jonshipman). If you change the author or the commit and repush the branch it should pick up the changes and cla signing.

@jonshipman
Copy link
Author

Signed.

@dmanners dmanners self-assigned this Jan 3, 2018
@dmanners dmanners added this to the January 2018 milestone Jan 3, 2018
@@ -235,7 +235,13 @@ protected function _getHtml(

if ($childLevel == 0 && $outermostClass) {
$outermostClassCode = ' class="' . $outermostClass . '" ';
$child->setClass($outermostClass);
$current_class = $child->getClass();
Copy link
Contributor

Choose a reason for hiding this comment

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

could you update this variable to be $currentClass

@magento-team magento-team merged commit 7c02acb into magento:2.2-develop Jan 7, 2018
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.

4 participants