Convert Navigation messages component to SDC

Everything looks much cleaner and simpler now!
Please take a look

i will have a look

It is a very nice SDC component! I love the facts you are not using Twig blocks fro your slot, iri-reference for URL prop, and both the definition and the template are very clean 👍

A few feedbacks however...

Little cleanups

Slots are not typed, so you need to remove "type" property:

slots:
  content:
    type: string
    description: Message content.

It seems you are using BEM methodology for the class naming, so this:

{%
  set classes = [
    'toolbar-message',
    'toolbar-message--type--' ~ type,
  ]
%}

Would be better written like:

{%
  set classes = [
    'toolbar-message',
    'toolbar-message--' ~ type,
  ]
%}

data-drupal-tooltip

Don't use a slot as an attribute value. A slot can have anything, including markup, and may break the browser DOM.

setAttribute('data-drupal-tooltip', content).

So, what can we do? The obvious options would be:

• Make content a string prop instead of a slot, especially if only plain text is expected here
• Create a new string prop for this attribute value

However, the best would be to remove those attributes from the template:

setAttribute('data-drupal-tooltip', content).setAttribute('data-drupal-tooltip-class', 'admin-toolbar__tooltip')

Because they tell us about the context how/where the component is used instead of being proper parts of the component.

I would inject them from outside (non tested proposal):

          '#type' => 'component',
          '#component' => 'navigation:message',
          '#props' => [
            'type' => 'warning',
            'url' => $url,
           'attributes' => new Attribute({
              'data-drupal-tooltip': content, # raw? processed? or something else?
              'data-drupal-tooltip-class': 'admin-toolbar__tooltip'
          })
          ],

Side note

I am recommending executing drush sdcv umami from https://www.drupal.org/project/sdc_devel/ to automatically get useful warning and errors.

There are still some rough edges (unclear messages, false positives...) but this tools is already usable.

Moved content to typed props.

Please review!

It looks good! Thanks a lot.

Some little proposals.

Definitions

Props:

• type title is missing
• content description is the same as the title, you can remove it

Templating

I am afraid {% if url is not empty %} is not catching when url value is missing. I would recommend the simpler {% if url %}

Thank you for quick response!
Fixed!

Tested this on an Umami install

Hate uploading duplicate files almost but here's before and after

After

Looking at the MR my only question was going to be if content should be a slot but see @pdureau already addressed that and I'll 100% defer to him on all things SDC.

Rest looked good to me.

@nod_ That dependency not needed anymore because CSS now provided in SDC

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed conflict in core/modules/navigation/src/Hook/NavigationHooks.php now that theme hooks moved to it's own.

I think we need to deprecate the theme hook if we can. Hopefully we can...

@alexpott

It's not entirely clear what we should deprecate!

The SDC component and theme hook can work without Umami.
Even if Umami is disabled, many modules may want to add messages to the Navigation.

https://www.drupal.org/project/drupal/issues/3441100#comment-15770404

That's why we implemented the styles and functionality in the Navigation module and not in Umami.

Or did you mean something else? Please clarify!

@finnsky when I made that comment this issue was removing stuff from \Drupal\navigation\Hook\NavigationThemeHooks::theme() - now it is not. The reason for this change is not explained in any of the recent comments... anyhow removing something from \Drupal\navigation\Hook\NavigationThemeHooks::theme() means that contrib and custom can have dead code that we need to tell people to remove.

I think this misunderstanding arose because the hooks were previously reworked, causing a merge conflict.

So I'm sending it back to review because the hooks aren't currently being changed in the MR.

As discussed in Slack
https://drupal.slack.com/archives/C7AB68LJV/p1763900832081709

removed navigation__message theme hook and added CR
https://www.drupal.org/node/3559492

Think the CR could use some love as that doesn't really seem to describe the impact.

I peaked the slack conversation @alexpott you mentioned "it is beta though" not sure if you meant core or the navigation module but navigation is marked stable so would this be a BC.

I'm not so sure.
Should we update the CR or add the theme_hook handler we removed and issue a deprecation message?

Not entirely sure especially with 11.3 actually releasing today.

I rebased and updated the CR.

Please check.

Rebased

This one has taken a lot of twist and turns but I believe the feedback has been addressed. Know navigation is still not marked stable so should be safe to do? Going out on a limb.

Committed and pushed 3b4e8c1c66c to main and e39714281b7 to 11.x. Thanks!

Correct version and branch on the change record.