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

Merge Requests

More

Recent comments

πŸ‡·πŸ‡ΈSerbia finnsky

One more thing

Many templates will shine as SDC components because they are reusable well-defined UI objects: status-messages.html.twig, breadcrumb.html.twig, table.html.twig, image.html.twig, progress-bar.html.twig, pager.html.twig, menu.html.twig... Let's focus on them.

The listed things are not so much needed as components.
They are usually written once and do not often need to be changed within the same site.

Unlike Card, Teaser, Button, Grid and other workhorses of any site. And this is
node.html.twig, user.html.twig, field.html.twig, block.html.twig, views.html.twig

πŸ‡·πŸ‡ΈSerbia finnsky

@catch please take a look at #15 #16

https://www.drupal.org/project/drupal/issues/3444525#comment-15689724 🌱 Compatibility of SDC use with display/experience builders Active

Let's discuss first at least.

πŸ‡·πŸ‡ΈSerbia finnsky

I thought today and would also like to discuss some points in the IS and comments.

So, because the call to this component is now hardcoded

Not now. It's worked this way since Olivero's beginning.
Now there is an SDC layer that greatly simplifies and improves the styles of the teaser and potentially other components (I want to use it for comments).

But in general, rolling back that Olivero teaser task will not change anything for those modules from the list.

hardcoded

I can't call it hardcode.
This is a completely valid and the ONLY way (for now) (unfortunately) in the core to implement components.
Those methods that require manipulation in the admin panel and complex config synchronisation may also not be liked by all members of the community.
I will be very glad when core can offer something different for components.
But this moment obviously has not yet arrived.

It would be a pity if Olivero, the default Drupal theme, is becoming incompatible with the strategy of targeting the ambitious site builders by improving the display building experience.

I personally don't like that Olivero doesn't always strictly follow BEM.
Contains unnecessary style cascades and so on.
I also believe that the developers of these ambitious initiatives are very strong and they will be able to rewrite and adapt any template.
And it will be much easier for them to do this if it will be good component from a front-end point of view. Even if implemented through Twig.

now have 2 layers of wrappers without gaining any benefit

Benefit is clean frontend. Smaller flat tree.

I don't see any situation where SDC is winning here.

It is not win of SDC but win of frontend component development approach. SDC is tool here.

But we are not there yet and it may be hard for the SDC enthusiasts and early adopters to project themselves into a future where we use SDC components mainly from render arrays instead of from Twig templates.

Nobody can predict the future. Not only SDC (actually BEM) enthusiasts.
And so let's use the only thing we have for now. Until, of course, the best came along.

So I consider the current issue at least controversial.
And definitely at this stage we should not prohibit anything or limit the development of components in any way.

πŸ‡·πŸ‡ΈSerbia finnsky

It was already a problem before SDC adoption, but SDC is putting a spotlight on it and may make the situation worse:

In other hand we have CSS and javascript better. So in my point of view every decision and especially restrictions should be discussed.

@catch in the ticket you just reopened
1. CSS became better.
2. Still exists ability to reuse this template for comment twig.
3. a bit more complicated twig but more structured.

πŸ‡·πŸ‡ΈSerbia finnsky

Why do we have markup for the image in the sdc template, then pull that back out into the node template?

it is optional slot. it can be presented or not.

πŸ‡·πŸ‡ΈSerbia finnsky

I've added one small comment. Could you please take a look?

πŸ‡·πŸ‡ΈSerbia finnsky

Fixed tests. Maybe not in good way.

Now ready for review in terms of frontend.

@joachim, please take a look if you want to adapt render elements to current backend.

πŸ‡·πŸ‡ΈSerbia finnsky

@andy
thank you for review.
gonna check now.
next time you also can add direct comment here. since gitlab only comment not updates issue status.

πŸ‡·πŸ‡ΈSerbia finnsky

Cool! I just removed polyfill from toggletip
https://www.drupal.org/project/drupal/issues/3197758 ✨ Create a new component: Toggletip RTBC

πŸ‡·πŸ‡ΈSerbia finnsky

Removed popover polyfill, it seems not required anymore

πŸ‡·πŸ‡ΈSerbia finnsky

yeah, i had it aswell. fixed in last commit

πŸ‡·πŸ‡ΈSerbia finnsky

Thanks for working on this annoying problem.
I looked and confirm that everything works well.

In the future I would remove the timeouts and replace them with animation times. But for now we can move on like this.

πŸ‡·πŸ‡ΈSerbia finnsky

Really not sure what the reason of failures. It should work. Probably filterVisibleElements() works incorrect.

πŸ‡·πŸ‡ΈSerbia finnsky

Yeah. Thank you. I meant ready on front side. Can be any adaptation on backend.

πŸ‡·πŸ‡ΈSerbia finnsky

Some weird functional js errors. Cannot reproduce them locally. And it is mostly ready.

I suggest to postpone views popup filters cause it has extra selectbox filter.

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 3460979-reduce-jquery-usage to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

I've added one comment here. Probably it will help

πŸ‡·πŸ‡ΈSerbia finnsky

Removed format for now. We need to configure linter + prettier for css and other files.

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 3452828-eslint-prettier to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

I have weird deja vu :)

https://www.drupal.org/project/drupal/issues/3409048#comment-15525622 πŸ› Configure postcss formatting after stylelint and stylelint-config-standard update Fixed
Linter issues aren't rebase friendly.

I will reapply changes in new 0.x based branch and let's create followup for CSS and MD

πŸ‡·πŸ‡ΈSerbia finnsky

Imo this is obvious not this issue false.
I think we can send it to review and open SDC ticket about it.

πŸ‡·πŸ‡ΈSerbia finnsky

illustrate ^

πŸ‡·πŸ‡ΈSerbia finnsky

I've found interesting thing here.

When module enabled and cache not cleaned yet.
toolbar-button.css not loaded only in context of claro theme. when i go on homepage with standart install profile (olivero).
Toolbar button works as expected. When move back to claro css not loaded again

πŸ‡·πŸ‡ΈSerbia finnsky

Fixed footer which was reported for me by Axe devtools.

πŸ‡·πŸ‡ΈSerbia finnsky

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

πŸ‡·πŸ‡ΈSerbia finnsky

I've added couple of feedbacks

πŸ‡·πŸ‡ΈSerbia finnsky

Fixed @pdureau feedbacks.
Keeping in NW status to check caching issue from #27

πŸ‡·πŸ‡ΈSerbia finnsky

It is weird but seems that until cache clean SDC doesn't load toolbar-button css

core/modules/navigation/components/toolbar-button/toolbar-button.css

I reproduced issue locally and in tugboat instance

πŸ‡·πŸ‡ΈSerbia finnsky

Gonna revert some css to fix bug with long text

https://gyazo.com/99dfdeeaf0ff3fcf82665da760e02b8a

πŸ‡·πŸ‡ΈSerbia finnsky

Fixed feedbacks. Thank you for review

πŸ‡·πŸ‡ΈSerbia finnsky

I've started with different approach here.
First of all i cleaned all buttons. To make them more component friendly.

1. Moved safe triangle to own component
2. Cleaned CSS.
3. Transformed component props to make them more flexible.
4. Forced all elements on Navigation to use existing template.
5. Simplified cascades for collapsible variant
6. And after all that preparations i moved component to SDC.

Looks and works cool. Please review.

πŸ‡·πŸ‡ΈSerbia finnsky

If possible i would like to postpone it.

πŸ‡·πŸ‡ΈSerbia finnsky

I believe that SDC provides an excellent opportunity to clean up what was done quickly and sometimes roughly.
Therefore, I believe that in this task we should not do β€œthe same thing only SDC”

We can rework and simplify this component. Remove quick and dubious decisions

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 3458215-migrate-toolbar-button to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

I've tested it. Works fine. I see other js errors in console. But seems unrelated.

πŸ‡·πŸ‡ΈSerbia finnsky

I've created new PR.

- It should only affect js changes.
- We need to create follow up issue for stylelint and yaml etc. Because current `npm run format` still fixes public and MD and CSS files.

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 3452828-prettier to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

I also see existing `"format": "prettier --write .",` task in package.json

Should we keep it?

πŸ‡·πŸ‡ΈSerbia finnsky

I've found MR contains some unexpected changes.
Like src/App.tsx

Also lint:fix show failures.

πŸ‡·πŸ‡ΈSerbia finnsky

Imo simplest option here will be always expanded sidebar on that page(on desktop). ignoring user setting.

πŸ‡·πŸ‡ΈSerbia finnsky

I still see imported variables in
core/themes/claro/css/theme/contextual.icons.theme.css
core/themes/claro/css/theme/contextual.theme.css

and some unexpected changes in core/themes/claro/css/theme/media-library.css

πŸ‡·πŸ‡ΈSerbia finnsky

Hello! I agree and understand role. Thank you!

πŸ‡·πŸ‡ΈSerbia finnsky

I've added one comment

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 11.x-3457061-modal-dialog to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

Yes. Core has Backward compatibility for jQuery listeners but not for jQuery triggers.
It seems pretty weird to expect that events will work same if they triggered differently than in core.

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 3457776-update-events-triggers to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

Imo this should be done on level of module. It trigger jQuery events. But in fact they should be vanilla events.
Opened own issue
https://www.drupal.org/project/bootstrap4_modal/issues/3457776 ✨ Update events triggers Active

πŸ‡·πŸ‡ΈSerbia finnsky

With Bootstrap 4 modal no issues when anonymous user
https://gyazo.com/ce11b077b994b2fd529d4b0575f0119d

Some issues when admin. Digging..

πŸ‡·πŸ‡ΈSerbia finnsky

Which bootstrap theme i have to test with?
Not even one of them cannot be applied on 11.x

πŸ‡·πŸ‡ΈSerbia finnsky

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

πŸ‡·πŸ‡ΈSerbia finnsky

I would say it differently.

At one time, Bootstrap 3 was an excellent framework that greatly moved the industry forward.
But now some of his decisions are slowing down this movement.
Such as a font size of 10 pixels for the root element for example.

It's time to say goodnight sweet prince...

πŸ‡·πŸ‡ΈSerbia finnsky

marking as bug because it's an a11y regression

We not sure yet.

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 3452724-add-back-the to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

We just removed that from Twig in linked ticket. In current ticket we need to decide if that title needed or not.
And if yes we need to add this with JS because of caching reasons.

Also hat titles were added initially from first days of development. And then we created others on block level. So probably they are artefacts and can be ignored.

πŸ‡·πŸ‡ΈSerbia finnsky

Added some comments. Take a look.

Also please. Why MR modifies core/misc/jquery.form.js ?

πŸ‡·πŸ‡ΈSerbia finnsky

Now seems all works as expected.
Please review.

πŸ‡·πŸ‡ΈSerbia finnsky

Some issues with RTL, but it close to finish now.

πŸ‡·πŸ‡ΈSerbia finnsky

Hidden outdated branches.

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch 3197758-create-a-new to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

finnsky β†’ changed the visibility of the branch toggletip-11x to hidden.

πŸ‡·πŸ‡ΈSerbia finnsky

Looks good to me, checked on all screen sizes and rem definitions.

Fixed 2 small bugs

1. scope of mobile expand button. now it is control-bar instead of top-bar
2. duplication of --admin-toolbar-top-bar-height variable in control-bar

That small bugs so i believe they can be applied as RTBC.

πŸ‡·πŸ‡ΈSerbia finnsky

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

πŸ‡·πŸ‡ΈSerbia finnsky

I think it may need change record since we add new function in Drupal Twig here.

Production build 0.69.0 2024