Account created on 22 November 2013, about 11 years ago
  • Front-end developer at Skilld 
#

Merge Requests

More

Recent comments

🇷🇸Serbia finnsky

Then yeah. We have tiicket to backport all to sdc

🇷🇸Serbia finnsky

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-...

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

Could you please add some test cases?

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

rebased and updated versions

🇷🇸Serbia finnsky

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 ;)

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

@mogtofu33 thank you for this and for all that initiative

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia 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?

🇷🇸Serbia finnsky

I don't think we should define viewbox somewhere

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

Thank for commit.
I added few notes about js.

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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?

🇷🇸Serbia finnsky

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()

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

Looks better imo ;)

Thank you for quick response

🇷🇸Serbia finnsky

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

🇷🇸Serbia 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

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

Sending to review. Plus a11y tag.

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

🇷🇸Serbia finnsky

What about second suggestion?
Can some schemes which

we want them to stay

be covered by schema?

🇷🇸Serbia finnsky

@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.

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

@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?

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

@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!

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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?

🇷🇸Serbia finnsky

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.

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

@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

🇷🇸Serbia finnsky

@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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

We still have dropdown with same trim spaces

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

@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...

🇷🇸Serbia finnsky

yeah. it `may` work` with couple of extra selectors. we need to think about it.

🇷🇸Serbia finnsky

it was added as far as i remember.
better to test for sure.
it was added in January :)

100% it was added into layout builder filter.

Production build 0.71.5 2024