🇷🇸Serbia @finnsky

Fix works for me.
Thank you!

I apologize.

I read the task description poorly.

As for MR
JavaScript looks acceptable to me.
+1 RTBC

No, I mean we can search in the module history.
These links have already worked with the dropdown list. And there is already a script and styles for them.
The backend for this module has changed several times recently, so we should just re-apply it.
I think js is not needed here at all. Maybe only for optimisation.

Hello! Thank you for work here.

Initially we managed that dropdown via common abstract script:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/navig...

And it worked before. We need to check git history here probably to avoid duplications in common things
This Dropdown should be universal(not related to contextual links) and reusable.

finnsky → created an issue.

nod_ → credited finnsky → .

Then yeah. We have tiicket to backport all to sdc

Probably some Microfrontends with common context. Just brainstorming. I don't have lot info about ExB.

https://dev.to/nik-bogachenkov/building-micro-frontends-with-vite-react-...

Hello!
We have that POC in https://www.drupal.org/project/drupal/issues/3401826#comment-15837369 🌱 [PLAN] Top contextual bar Active

Applied styles.

I'm not really sure how to apply styles to nothing :)

Could you please add some test cases?

Radix components also can be SDC :) And folow JSON schema definitions.

rebased and updated versions

I want to suggest this one for SDC components.
It is still in development, but already supports lot of things
https://www.npmjs.com/package/storybook-addon-sdc

Check Readme ;)

Thank you for work here.

1. We had something like this in initial versions of design. Then it was replaced with current variant. So i added required label.

2. Visual regression in focus ring https://gyazo.com/5b8989e769ebf73709d00bf94f2e0004

3. Keyboard navigation seems not worked as before. EG: Right and left arrow behaviour.

4. Also small note in MR

This button still active in development.

https://git.drupalcode.org/project/drupal/-/merge_requests/9116/diffs#1b...
and others
So its override may lead us to lost some features.

There are many ways to break the navigation module. But on the other hand, the example you gave can break many other integrations

button {
  display: inline-block;
}

It violates at least two basic BEM rules:

1. Uses a tag as a selector
https://en.bem.info/methodology/css/#selectors

2. Adds external geometry to the block
https://en.bem.info/methodology/css/#external-geometry-and-positioning

The basic reset work was done here, but we are open to suggestions on how else we can preserve good components in the navigation. There are no web components in the core yet.
https://www.drupal.org/project/drupal/issues/3446357 🐛 Buttons are not accessible with display: contents applied Active

kristen pol → credited finnsky → .

https://drupal.slack.com/archives/C1BMUQ9U6/p1730889311131359

This issue has POC with styles required
https://www.drupal.org/project/drupal/issues/3401826 🌱 [PLAN] Top contextual bar Active

Thank you for you work here.
I found that this problem a bit deeper than it seems from first view. Added few comments. Not sure yet what to do with this

@mogtofu33 thank you for this and for all that initiative

In addition to #57

Please check this comment https://www.drupal.org/project/drupal/issues/3483209#comment-15841982 📌 Navigation leverage icon core API Needs work

I think we should keep the original viewBox attribute if it is not reassigned by the template

Here I added a simple illustration of how the absence of the original attribute breaks the adaptability of icons.

https://codepen.io/finnsky/pen/jOgKdWz

griffynh → credited finnsky → .

While i research adaptation on https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work issue.
I found one big problem of this template approach.
We shouldn't define viewBox in template.
It is something that belong to svg itself and this attribute makes svg responsive.

It is possible to use original viewBox from original SVG?

I don't think we should define viewbox somewhere

Thank you for great start. I see some bem mistakes. Gonna fix it.

finnsky → made their first commit to this issue’s fork.

Reverted last commit

Thank for commit.
I added few notes about js.

I understand that we shouldn't add MR here. But in order to get things moving I'll try.

I added a few areas as suggested. Created a few new SDC components.

Now it looks like this

We need to add an understanding of how modules can extend these areas. (I honestly still don't have it). And add subtasks for these areas/components.

Fixed icon plus added common tooltip
https://gyazo.com/795757bee733d175de17e7815f40cc64

I don't think that in this issue we should change core code

I think it still requires some prove label in title ;)
I see now real chaos in titles
https://gyazo.com/0f847b120acbbc46226ddfc0cf47815e
we have h2 and h4. which is real title here? and which one should be aria label?

I think it is ok about button positioning with js.
But we need to clean different position calculations for button.
And move them to css:has()

i fixed js but seems we need to care about reload again
https://gyazo.com/e1a88c9d32cbd215f97a11b6198d92b6

also keyboard search broken because button still has data-index-text="u". not sure it is critical. maybe we also need to move that functional out of twig to js

js not works
https://gyazo.com/f93eb21f5e7ddba4797e2cc9292d056f

finnsky → made their first commit to this issue’s fork.

Thank you!

CSS changes looks good to me.
Also tested basically and didn't found visual regressions.

+1 RTBC. Would be cool if anyone can make before/after screenshots.

finnsky → changed the visibility of the branch 3303546-claro-dialog-refactor-d11 to hidden.

Getting tonns of errors like

[Tue Oct 22 20:50:22 2024] Uncaught PHP Exception Drupal\Core\Render\Component\Exception\InvalidComponentException: "[props.properties.tags.items.properties.attributes.type] Does not have a value in the enumeration ["array","boolean","integer","null","number","object","string"]/n[props.properties.tags.items.properties.attributes.type] String value found, but an array is required/n[props.properties.tags.items.properties.attributes.type] Failed to match at least one schema/n[props.properties.tags.items] Object value found, but an array is required/n[props.properties.tags.items] Failed to match at least one schema" at /Users/ivan/Sites/experiments/navi-core/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php line 121
[Tue Oct 22 20:50:22 2024] [::1]:49327 Closing

when defining attributes in component. via object or that weird Drupal related type

Also i hope soon icons will appear as configurable entities in Navigation.

Looks better imo ;)

Thank you for quick response

Parent issue merged

I think any regular weight icon that you like. I'm not designer. I only can detect design mistakes :)

finnsky → created an issue.

finnsky → created an issue.

gábor hojtsy → credited finnsky → .

I've fixed all issues from #32

But one important thing.

We have yet another issue. https://www.drupal.org/project/drupal/issues/3441100 📌 Integrate Umami message Needs review

Where we add messages into navigation via theme hook(s).
I would like to get backender review of both issues. to get best and simplest way to extend navigation.

Here for example we ignore that buttons(messages) can be displayed as list.

So before we will extend Navigation by other modules let's have universal extending mechanism

@crina could you please define animation icon
https://gyazo.com/86a2d286fe3d5bdb54d75e7619f6f107

Also should this block be also configurable from UI?
Like
https://gyazo.com/71f405fd6ea357cc707056a5c3fc8bde

Points 1, 2 and 3 from #32 are still outstanding

Yes. But scope of the current issue is navigation module. So i think we should fix it here aswell.

Move to SDC became blocker because in current task we reworked lot of things from first versions, which sometimes were done quickly

yes, i've tested in both modes

I still think this question is deeper. And it is not duplication of existing issues.

this subject is broader than UIP2, because Experience Builder is also using a PHP stream wrappers

doesn't mean that this is standard ;)

Now SDC from UI Patterns not compatible with SDC from experience builder and both of them cannot be used outside of that modules.

I've added third suggestion.

We want to make an enum for icons but we all know that in the end it will be an icon from the backend.
I see this as a contradiction and this contradiction is not worth continuing to block this important task.

That's why I removed the enum from icons. (If you want to return it, you need to rewrite the tests)

I also added detach.

Please review

I've added fix but i don't really like it. I would better avoid changes on core level in this MR. We can do this fix later. When we get more info

Sending to review. Plus a11y tag.

On my side axe report is fine with message on page.

What about second suggestion?
Can some schemes which

we want them to stay

be covered by schema?

@nayana_mvr
What can we do here?
The best thing is to add an instantly executable script(library) that will have no dependencies and will have the smallest weight. And doing the same thing that the inline script does now. It doesn't need Drupal or once. It even can start earlier than navigation is rendered.

finnsky → created an issue.

but why repeating toolbar-button-- in every value:

i don't thin we should really care about repeating here. but ok.

Are you sure every modifier can be combined with every other modifier? Because your model makes it possible and this need to be checked.

yes. they are designed so.

expand--down and expand--side. Do that mean it can be its own expand prop?

it is just css selector at the moment. probably later.

weight--400 but there is only one value. Do you expect more later?

yes. for now we have various buttons only in navigation and one in top bar.
https://gyazo.com/cdaff6debbde93c95747684b60e4a8a5
but should be much more later

Please review.

Moreover, it seems to me that here we do it much more elegantly than in the attached task. Using progressive but at the same time already well-documented technology.
https://developer.mozilla.org/en-US/docs/Web/API/Web_components

So after testing here, in my opinion, we should move this to the core in linked task

@catch.

Pushing it to core will be long and complicated. In same time Umami is experimental. Let's do it here and link it in that issue?

Recreated MR ro review this as Webcomponent only, without previous context.

I found original bug with safari.
https://github.com/hotwired/turbo/issues/1245

If i just create node with button and popover it works. But if it is inside any form. it doesn't work in safari

finnsky → changed the visibility of the branch toggletip-11x-separate-components to active.

finnsky → changed the visibility of the branch toggletip-11x-separate-components to hidden.

Seems some php linters were changed.
I have no power here ;)

@nod_ thank you for feedbacks. i fixed everything according them.

I agree - we need to care about jquery selectors in own task and deprecate them globally at once.

please review!

Rebased. Please review.

You might want to change those data-foo keys at some point

It can be managed with setting data attributes with json _encode. It exists in lot of places in core in dialog options. And read them with JSON.parse() on frontend.

          'data-dialog-options' => Json::encode([
            'width' => 555,
            'classes' => [
              "ui-dialog" => "ui-corner-all top-1",
            ],
          ]),

After we release we should never change attributes without deprecation messages.
Technically it is still input type="search". with some extra attributes. Still need reason to have that abstract layer.

Please take a look and confirm or deny

It seems to me that there is not much difference between:

    $form['filters']['text'] = [
      '#type' => 'search',
      '#title' => $this->t('Filter modules'),
      '#title_display' => 'invisible',
      '#size' => 30,
      '#placeholder' => $this->t('Filter by name or description'),
      '#description' => $this->t('Enter a part of the module name or description'),
      '#attributes' => [
        'class' => ['table-filter-text'],
        'data-table' => '[data-drupal-selector="system-modules"]',
        'data-items' => '.package-listing table tbody tr',
        'data-targets' => '.table-filter-text-source, .module-name, .module-description',
        'data-singular' => $this->t('1 module is available in the modified list.'),
        'data-plural' => $this->t('@count modules are available in the modified list.'),
        'data-full' => $this->t('All available modules are listed.'),
        'data-search-start' => 'true',
        'autocomplete' => 'off',
      ],
    ];

AND

    $form['filters']['text'] = [
      '#type' => 'list_filter',
      '#title' => $this->t('Filter modules'),
      '#title_display' => 'invisible',
      '#size' => 30,
      '#placeholder' => $this->t('Filter by name or description'),
      '#description' => $this->t('Enter a part of the module name or description'),
      '#list_container_selector' => '[data-drupal-selector="system-modules"]',
      '#list_item' => '.package-listing table tbody tr',
      '#list_text' => '.table-filter-text-source, .module-name, .module-description',
      '#search_start_of_words' => TRUE,
      '#announce' => [
        'singular' => $this->t('1 module is available in the modified list.'),
        'plural' => $this->t('@count modules are available in the modified list.'),
        'all' => $this->t('All available modules are listed.'),
      ],
      '#attributes' => [
        'autocomplete' => 'off',
      ],
    ];

for a developer.

And this render element seems to be just an extra layer. Which appeared without any real need. What does this give us?

I thought to release first version.
With basic features. And then after tests extend it with views with selectbox etc. Otherwise we will never finish it.

rebased

I've added one comment about jQuery selectors.
All other feedbacks are fixed.

Probably this custom filter can be moved to temporary module?
To make contributors life easier.
Core obviously doesn't need it. All regressions fixed

I've tested templates before/after patch.
WIth/without twig debug.
Templates without regressions marked with +

core/modules/block_content/templates/block-content-add-list.html.twig +
core/modules/ckeditor5/templates/ckeditor5-settings-toolbar.html.twig +

core/modules/link/templates/link-formatter-link-separate.html.twig
fixed(looks as before now)

core/modules/link/tests/themes/link_test_theme/templates/link-formatter-link-separate.html.twig
revered `-` signs there. Now works as before with/without twig debug

core/modules/system/templates/dropbutton-wrapper.html.twig +
core/modules/system/templates/select.html.twig +
core/modules/toolbar/templates/toolbar.html.twig +
core/profiles/demo_umami/themes/umami/templates/classy/field/link-formatter-link-separate.html.twig +
core/profiles/demo_umami/themes/umami/templates/classy/navigation/toolbar.html.twig +
core/themes/claro/templates/classy/field/link-formatter-link-separate.html.twig +
core/themes/claro/templates/classy/navigation/toolbar.html.twig +
core/themes/claro/templates/form/input.html.twig +
core/themes/claro/templates/navigation/toolbar.html.twig +
core/themes/claro/templates/views/views-mini-pager.html.twig +
core/themes/claro/templates/pager.html.twig +

core/themes/olivero/templates/content/node--teaser.html.twig
fixed visual regression with css
it related to core/modules/node/templates/field--node--uid.html.twig which i don't want to change here

core/themes/olivero/templates/content/node.html.twig
same CSS fix plus extra wrapper.

core/themes/olivero/templates/content/search-result.html.twig +
core/themes/olivero/templates/navigation/pager.html.twig +
core/themes/olivero/templates/user/username.html.twig +
core/themes/olivero/templates/views/views-mini-pager.html.twig +
core/themes/stable9/templates/admin/block-content-add-list.html.twig +

core/themes/stable9/templates/admin/block-content-add-list.html.twig +

core/themes/stable9/templates/field/link-formatter-link-separate.html.twig
fixed(looks as before now)

core/themes/stable9/templates/form/dropbutton-wrapper.html.twig +
core/themes/stable9/templates/form/select.html.twig +
core/themes/stable9/templates/navigation/toolbar.html.twig +

starterkit didn't checked. just reapplied fixes

@luke.leber

your #55 assumption is incorrect

{% set var = '<span>Hello</span> <span>There!</span>' %}
{{- var|raw -}} -> renders Hello There!

Spaces trimmed from begin and end

@nod
this is answer. thank you for review.
https://www.drupal.org/project/drupal/issues/3100083#comment-15692187 📌 Add js message theme override to match Umami message markup Needs review

Btw we can dramatically simplify everything by only using CSS :has() which is in current Drupal browserlist.

Great News! It will unblock tonns of issues here!!!

just did it.

We still have dropdown with same trim spaces

@bbrala, probably let's create followup issue here? And for now will test whitespace/newlines trimmed results?

seems `3 times` of random failures. at least gone after rebase

Fixed feedback and add small fix in case of wrong context of textarea.

@bbrala
please take a look at failed test output. can you tell me what is wrong? :)

https://issue.pages.drupalcode.org/-/drupal-3477375/-/jobs/2898571/artif...