Problem/Motivation

The top-bar-page-actions.html.twig renders the links in the kebab as provided, i.e. including extra attributes that are provided.

However, extra attributes for links in the featured page action list are ignored. This limits their capabilities a lot, i.e. they can't use ajax or other fancy stuff.

Proposed resolution

Add the attributes of featured links in the same way as for other links.

Issue fork drupal-3537382

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

jurgenhaas created an issue. See original summary.

jurgenhaas opened merge request !12810

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Small change but could we add an assertion to an existing test

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

I've added a test for this and it seems to be working OK. But the pipeline shows a PhpUnit error, I couldn't find out, though, what's wrong. The one failing test I found is scaffolding related and that isn't related to the MR.

I probably need some assistance in finding out what else is going wrong, if anything.

smustgrave’s picture

smustgrave commented

Can you re run?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Needs review

Oh, nice, that worked. Still a few tests running, but setting to NR since the relevant tests passed.

smustgrave’s picture

smustgrave commented

Yea if they fail repeatedly in a row I’d say not random but we do have those random ones that popup

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs review » Needs work

Back to NW, there's still something wrong, need to look into this tomorrow.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Needs review

Tests are now green, ready for review.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs Review Queue Initiative

Thanks for adding the test coverage

Ran the test-only feature

1) Drupal\Tests\navigation\Functional\NavigationTopBarTest::testTopBarVisibility
Behat\Mink\Exception\ElementHtmlException: The attribute "data-dialog-type" was not found in the element matching xpath "//div[contains(@class, 'top-bar__content')]/div[contains(@class, 'top-bar__actions')]/a[contains(@class, 'toolbar-button--icon--database')]".
/builds/issue/drupal-3537382/vendor/behat/mink/src/WebAssert.php:927
/builds/issue/drupal-3537382/vendor/behat/mink/src/WebAssert.php:625
/builds/issue/drupal-3537382/vendor/behat/mink/src/WebAssert.php:672
/builds/issue/drupal-3537382/core/modules/navigation/tests/src/Functional/NavigationTopBarTest.php:98
FAILURES!
Tests: 1, Assertions: 18, Failures: 1, PHPUnit Deprecations: 2.
Exiting with EXIT_CODE=1

What happens though if there is no #attributes?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Not sure why the test-only test fails, maybe it doesn't take all the provided changes into account? When the full test is being run, that test is green.

What happens though if there is no #attributes?

No change to how Drupal works without this MR. The twig code changes from create_attribute() to create_attribute(link['#attributes']|default([])), so it adds an attribute if present or defaults to nothing, which is what is the default now as well.

Does this require another test?

smustgrave’s picture

smustgrave commented

Test only fail is good shows the test coverage works

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

That's what I get for having too many tabs open. I thought I saw ['#attributes'][another_key] I see now. yea |default([]) works.

That was my only comment, rest looks good

nod_ closed merge request !12810

  • nod_ committed 8d86de67 on 11.2.x 
    Issue #3537382 by jurgenhaas, smustgrave: Link attributes in top bar are...

  • nod_ committed 61e09f28 on 11.x 
    Issue #3537382 by jurgenhaas, smustgrave: Link attributes in top bar are...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 61e09f28155 to 11.x and 8d86de67efe to 11.2.x. Thanks!

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

As a contributor, attribute any organization that 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.