Problem/Motivation

The problem that I faced was that after migrating from version 1.x to 2.x there were some missing CSS classes on which my theme was relying for styling the menu items.

In version 1.x inside the buildForSitemap function there was a $class variable which sets several classes, which are not present in the 2.x version.

Proposed resolution

I have created a patch, which adds the missing classes, but without the "first" and "last" classes, which are not needed for me.

Remaining tasks

  1. Write a patch - done by @divanova in #2
  2. Add a test - done by @tom konda by #7
  3. Review and feedback - done by @mparker17 in #8
  4. RTBC and feedback - done by @mparker17 in #8
  5. Commit - done by @mparker17 in #9
  6. Release - released in 8.x-2.4

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#2 missing_css_classes_8.x.2.0.patch1.58 KBdivanova

Issue fork sitemap-3400050

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

divanova created an issue. See original summary.

divanova’s picture

divanova commented
StatusFileSize
new missing_css_classes_8.x.2.0.patch1.58 KB
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Needs review » Needs work

@divanova, thank you very much for the contributon; and thank you for your patience with me!

This patch looks great: the only thing that's preventing me from merging it is automated tests. I need automated tests, because I don't want future changes to the sitemap module to break this functionality, and break your website as a result!

I think you could do this by modifying \Drupal\Tests\sitemap\Functional\SitemapMenuTest::testMenus() to include some more assertions. For example, the following checks for an "expanded" class on an HTML <a> tag with the link text $node_1_title...

$this->assertEquals('expanded', $this->getSession()->getPage()->find('named', ['link', $node_1_title])->getAttribute('class'));

... note that example might not work (because I haven't checked it with the patch applied)... you'll have to look closer at the code in \Drupal\Tests\sitemap\Functional\SitemapMenuTest to see what it's supposed to do. You may also have to look at the the PHPUnit output files ("artifacts") to see what actually happens in the test environment.

Thank you again!

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue tags: -CSS classes +Needs tests

tom konda made their first commit to this issue’s fork.

tom konda opened merge request !54

tom konda’s picture

tom konda
Primary language Japanese
Location Kanagawa, Japan
commented
Status: Needs work » Needs review

Reroll patch #2 and add PHPUnit test.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thanks @tom konda!

This looks good to me! Manual testing shows this working well.

I couldn't get the active-trail class to display, even when I put the sitemap in a menu being output onto the sitemap - but the code to detect the active-trail existed before this patch (I think it's likely that I've set up my test site wrong) - I don't see any reason to hold back this change.

  • mparker17 committed d6063588 on 8.x-2.x authored by tom konda 
    [#3400050] feat: Missing css classes to menu items

By: divanova
By: tom...
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Merged! Thanks everyone!

I'll look into making a release with these changes in the next few weeks! (I'm hoping to merge #3545794: Remove unused cache, max_age configuration options in SitemapSyndicateBlock and #3545858: Migrate sitemap.settings.rss_front to SitemapSyndicateBlock's configuration; test, deprecate SitemapSyndicateBlock before making a release)

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Version: 8.x-2.0-beta4 » 8.x-2.x-dev
Issue summary: View changes

I've released this change in version 8.x-2.4!