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

Merge Requests

More

Recent comments

🇷🇸Serbia finnsky

@anjali rathod Thank you for joining Navigation!

You don't need to assign tasks to yourself. Usually it's enough to write that you're working on it.

https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

🇷🇸Serbia finnsky

Yes, it could be a good replacement but it seems that the library I suggested is no longer supported.
https://github.com/taye/interact.js

Also not clear to me how this interacts (or not) with (dialog)

It is related in such a way that we also have to replace or remove
https://jqueryui.com/resizable/
Here is an option with replace

🇷🇸Serbia finnsky

Rebased. Please review

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

Then perhaps it's worth considering why the dictionary throws exceptions for fairly popular properties that are fully supported by the core? I don't understand the purpose of the dictionary in this case

These properties are obviously not drupalisms.

🇷🇸Serbia finnsky

I don't think so, the drupal dictionary contains drupal specific things. While I want to add global CSS properties.

🇷🇸Serbia finnsky

I added quick implementation of
https://web.dev/articles/declarative-shadow-dom
Almost current Drupal browserslist https://caniuse.com/declarative-shadow-dom

Works as a charm. Toolbar button fixed ;)

🇷🇸Serbia finnsky

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

🇷🇸Serbia finnsky

Yes, this is a more accurate way of determining child nodes. + 1 to RTBC
Thank you!

🇷🇸Serbia finnsky

Fixed in related issue.
https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work

🇷🇸Serbia finnsky

Thank you all for participating!

🇷🇸Serbia finnsky

I just would like to keep properties names in title.

🇷🇸Serbia finnsky

I can't say that I agree with the wording.

Yes, these properties are used only by navigation so far. But even without navigation, it makes sense to add them since this progressive technology can be used by others.

🇷🇸Serbia finnsky

Thank you for review. Gonna check now.

🇷🇸Serbia finnsky

I rolled back the use of icons to the clean class.
Now everything works. And that problem from the video is no longer repeated.

Let's test!

🇷🇸Serbia finnsky

This video explains a lot ;)

I'll check it

🇷🇸Serbia finnsky

Thank you for work here.

I would like to ask for some improvements.

1. Use core lint-fix yarn task.
2. Try to avoid testing in css
3. Please avoid !important
4. Please check if it possible to fix outside of module components. It is not strict related with toolbar-menu component. So why that changes here?

🇷🇸Serbia finnsky

I want to thank everyone again for their enthusiasm!

But also to clarify that this MR already makes many useful improvements!

Let's do everything to get it merged as soon as possible and continue discussing the icon configuration in a separate issue.

https://git.drupalcode.org/project/drupal/-/merge_requests/11172
For example, there are several comments here that we should address here, or do later?

🇷🇸Serbia finnsky

Thank you!

The "Edit" button is read aloud as "Ed Edit" to screen readers

It should gone with this MR landed:
https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work

🇷🇸Serbia finnsky

We should focus on the Icon API here, and not see any visual regressions from the state of 11.x

We should not compare the appearance with version 11. But with the design that does not exist.

In fact, only the color and font thickness of the button are beyond the scope of this task. But this button is not in the design. What is currently in version 11 is something random. In addition, it was added incorrectly.

So I don't see anything critical if it is here.

🇷🇸Serbia finnsky

Thank you for starting review it.

2. The Top Bar edit button is regressed as well. It has the wrong icon now and the wrong font-weight.

They are actually structural fixes. Regressions were added previously.
Few of them out of scope of that ticket. But they should be done anyway.
https://www.drupal.org/project/drupal/issues/3505291 🐛 Undo some changes Active

🇷🇸Serbia finnsky

1. I prefer not to duplicate class functionality. We already have [class*="toolbar-button--icon"]
We should use it. Just add a check for the text attribute.

2. In #6 I pointed out that the close button on mobile is also broken.

🇷🇸Serbia finnsky

Well then let's leave it as is for now.

This MR already solves many problems on the frontend.
In the modules that use Navigation nothing will break and it will simply display the first two letters and the Icon can be added in CSS.
Which is quite flexible.

What is really needed here is good testing.

🇷🇸Serbia finnsky

CSS option for extending still exists. But maybe we can make it ideal ;)

🇷🇸Serbia finnsky

@mogtofu33, @pdureau

I have question, maybe you have idea?

How technically modules which extend Navigation may set their own icons with current approach?

i'm thinking about
modulename.icons.yaml
and
{{ icon(‘modulename’, icon, { class: ‘toolbar-button__icon’, size: 20 }) }}

It can be SDC option:
icons_lib:
default: navigation

Modules like: (before they had CSS option for this)

https://www.drupal.org/project/navigation_extra_tools

🇷🇸Serbia finnsky

I added a role and text for the button. Nw tests passed.

But a11y needs to be checked here.

And in general, what is top_bar? What is its role? Header or navigation?

🇷🇸Serbia finnsky

It is not small hot fix but reason is in wrong icons management in module.

https://gyazo.com/cfbb9da933c5d883bf840b02a9b6a522

All buttons with only one icon broken. Close on mobile as well.
I would like to fix that system problem in global linked ticket.

🇷🇸Serbia finnsky

+++ for initiative! that CSS logic is much better than original one. Better to check presence of elements and not :empty()

🇷🇸Serbia finnsky

https://www.drupal.org/project/drupal/issues/3476471 📌 Replace js message with webcomponents Active
Merged now. So it can be used as testing area for `template` and Shadow Dom approach

🇷🇸Serbia finnsky

Only one test failure:

2x smaller StylesheetBytes!!!

All tests green!

🇷🇸Serbia finnsky

I used the power of CSS

We couldn't do this before because we didn't have an element in DOM. But now we know that if there is no icon we will add an attribute. I think it's great

All that's left is to fix the tests. Yaay

🇷🇸Serbia finnsky

yeah, seems so we need to add checking if icon in existing icons list. this should be condition.

Is there something we need to change in the Icon API side?

Seems no. Looking good! Thank you!

🇷🇸Serbia finnsky

I looked at the MP! It looks great!

Here's what I added;

Since the icons have the same namespace, they can't be in different folders to avoid name collisions (spoiler: there was one already. Chevron in the button and menu item)
I moved all the icons to one folder

We can't use the toolbar-button--icon class for all the icons. Firstly, it doesn't follow the Bem style, and secondly, we need different classes. I added my own classes to the icons right in the config! Great!!!!

I removed the fill attribute from the icon. It's much better to set this from the CSS, especially since we have classes

Added icons to places where they weren't there yet. Different chevrons. Back button.

Corrected the styles and removed some hotfixes from recent tasks

Corrected the ajax animation

Corrected the messages

What needs to be done?

Need to fix tests

And most importantly:

If there is no icon, we should display the first two letters of the link name
https://gyazo.com/ed31b0f04e1d5ef87e278152bf1a98f1

In toolbar-button.twig
{% if icon is empty %} does not work
{% if icon|render is empty %} works

To check, add any link to the top level of the menu

🇷🇸Serbia finnsky

Then I would solve the testing problem in the next task. Here we have a critical bug.

I have no idea how the BigPipe works and how well it is handled by Nightwatch. But now the Navigation module does not work at all in Olivero and partly in Claro.

🇷🇸Serbia finnsky

@smustgrave i'm not sure. what you mean? tests green. we need review here

🇷🇸Serbia finnsky

We need to pause icons tickets and concentrate on Icon API implementation

https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work

🇷🇸Serbia finnsky

Messages appear in Umami demo profile only for now.
https://www.drupal.org/project/drupal/issues/3441100#comment-15984062 📌 Integrate Umami message Active

🇷🇸Serbia finnsky

Hello! Thank you for working on it.

Could you please create MR here as described in https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...
It will be easier to review and test.

🇷🇸Serbia finnsky

Thank you very much!
This is great step!

We also need to clean up some css tricks which we did for iconless buttons. I'll care about it in next days.

🇷🇸Serbia finnsky

@mogtofu33 hello!
It is possible to update that MR? Would be cool to have it onboard before stable release

🇷🇸Serbia finnsky

Pushed some css which make work done.

I would like to have some js optimisations here aswell

🇷🇸Serbia finnsky

finnsky changed the visibility of the branch 3505291-undo-some-changes to hidden.

🇷🇸Serbia finnsky

finnsky changed the visibility of the branch 3365377-metadata to active.

🇷🇸Serbia finnsky

Rebased. Thank you Please review.

🇷🇸Serbia finnsky

One backend fix need to be applied here. We can go with here.

🇷🇸Serbia finnsky

I was saying that these headings were there from the very beginning of the project and I don't think anyone ever seriously looked into the semantics or accessibility of the headings. That's why we removed them.

What we really need is a quality review of accessibility and structure. And depending on the review results, add or remove headings. I'm not sure they are needed at all.

@ckrina wrote in #22

Removing the "prove the need" because it's proven already that this is necessary to fix #3438976: [PP-2] Implement a caching strategy for the menu links. Discussing this with @plopesc.

But I don't understand how caching is related to accessibility and semantics

🇷🇸Serbia finnsky

Prove the need

was in the title not because of caching but because of a11y.

🇷🇸Serbia finnsky

@vladimiraus thank you for care about this issue ;)

@smustgrave here usual random failures. all green now.

🇷🇸Serbia finnsky

Follow up issue
https://www.drupal.org/project/drupal/issues/3504265 🐛 Yarn watch task broken Active

🇷🇸Serbia finnsky

finnsky changed the visibility of the branch 3504265-yarn-watch-task to hidden.

🇷🇸Serbia finnsky

Updated according https://github.com/paulmillr/chokidar?tab=readme-ov-file#upgrading

added usePolling: true because on macos i had lot of errors like:

yarn run watch:css
node:internal/fs/watchers:207
      const error = new UVException({
                    ^

Error: EMFILE: too many open files, watch
    at FSWatcher._handle.onchange 
🇷🇸Serbia finnsky

we upgrading node packages to major versions without reading changelog ;)
https://github.com/paulmillr/chokidar?tab=readme-ov-file#upgrading

it is broken now core/scripts/css/postcss-watch.js

const fileMatch = './**/*.pcss.css';
// Ignore everything in node_modules
const watcher = chokidar.watch(fileMatch, {
  ignoreInitial: true,
  ignored: './node_modules/**'
});

so for 1.5 month `yarn watch` not working ;)

🇷🇸Serbia finnsky

finnsky changed the visibility of the branch 3476471-replace-js-and to active.

🇷🇸Serbia finnsky

finnsky changed the visibility of the branch 3476471-replace-js-and to hidden.

🇷🇸Serbia finnsky

Thank you for review.

I added a disclaimer. Not sure what form it should take. But I tried to be formal and bureaucratic :)

Please take a look!

🇷🇸Serbia finnsky

Perhaps we should have done it through the SDC right away. But it is quite possible to implement it through subsequent tasks.

🇷🇸Serbia finnsky

@hetal.solanki
no need to assign tickets.
it is enough to write that you work on it.

🇷🇸Serbia finnsky

At the moment, the module is still experimental.
We are still waiting for the final design.
We are still waiting for the icon system.

The task description says

This issue is to ONLY move the Edit button outside of the drop-down, not applying any of the other visual changes to the image above is providing.

So there is no need to make it fully consistent with the image. It is enough to just add the markup and roughly understand how it will work afterwards.

And even more so, there is no need to add deeply nested CSS that will be impossible to clean up later without significant effort

Also, there is no need to add new icons. In particular, the Pencil was already there. Let it be different, but there will not be many icons with the same meaning when we switch to the icon API. (I hope soon)
I also took the icon of three dots from https://phosphoricons.com/ where all the other icons came from.

I also think that there is no point in adding backend tests based on visual styles, for example, the icon type, but this is not my area and I did not change it.

@plopesc could you please review backend after rebase?

🇷🇸Serbia finnsky

A few notes:

It shouldn't be a list because it's not a list.

It also breaks the output in some themes (Claro) because the item list has its own styles https://gyazo.com/4667c856e63d3d4b3459b2ab0c43b264

We should keep in mind that everything we create should (and will) be reused in the further development of the module. When we make forms and the second sidebar, for example.

And these two components: title and badge are key elements of any design system. Therefore, it is obvious to me that they have no place in the depths of the nested CSS

I remade it as I see the further development of the module components.

Result:
https://gyazo.com/cda07be66460fdec1d06fb635e99f0e2

Several fixes are needed in the tests and possibly the render array.
Please review

🇷🇸Serbia finnsky

Rebased. Please review

🇷🇸Serbia finnsky

It still seems to me that in this task it is much more important to understand why the script is loaded in the wrong place than to fix the script directly. This is also a bug and a much more serious bug.

@grimreaper,
Maybe you have steps to reproduce? A list of third-party modules and some other details?

🇷🇸Serbia finnsky

Thank you for fix. Actually it is interesting why toolbar js loaded for anonymous.

🇷🇸Serbia finnsky

Thank you. I added comment about nesting. Please check and compare with existing CSS of module.

🇷🇸Serbia finnsky

Probably we will need to backport valuable CSS/JS changes from this MR then.
Currently we have bug with anything displaced from top.
Not only workspaces.

Also Ajax issues from #32 also global. So this prio maybe higher.

🇷🇸Serbia finnsky

I would like to have it merged. It has some improvements about buttons and events.
On my side RTBC + 1

🇷🇸Serbia finnsky

Fix works for me.
Thank you!

Production build 0.71.5 2024