πŸ‡·πŸ‡ΊRussia @kostyashupenko

Omsk
Account created on 12 August 2015, almost 9 years ago
  • Senior front-dev at SkilldΒ 
#

Merge Requests

More

Recent comments

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Here https://git.drupalcode.org/project/component_connector/-/blob/1.x/src/Co...

we have a problem with components integrated using `Suggestion` type. Auto-loading not working because of `.suggestion` suffix is in $asset name.

Here is example of dump of $asset:

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Good catch. I can reproduce layout shifting and wrong position of Close button only when `Navigation` module is enabled.

Can be reproduced on screen width is ~1034px

`--drupal-displace-offset-left` is calculating correctly. The real reason of all that is this line https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/oliver...

But if to remove this line - we'll get the same bug if `Navigation` module isn't installed (so with default Toolbar).

For now no solution for this, but at least issue summary should be updated

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I can't find solution for this, maybe some hints?

Maybe we can replace CSS mask-image with embedded svg icons ? Or to use iconic font and content css property (but embedded technology is better)

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Good catch.

It's now ready for review.
made also span element not interactable visually (no hover styles). Also added few fixes for forced-colors: active mode

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

RE #10
i'm attaching correct gif with the results

btw @Gauravvvv i think it is not required to change those px values, because if you will look to the context of the selector you will understand that the styles are used to hide visually an element.

Keeping on Needs review still

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

MR is ready for review. In #5 i have uploaded wrong gif with the demonstration of results. Will upload correct gif later, but you can test in any way.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I think we have to keep rems.

Themes and modules can conrol everything by overriding --admin-toolbar-rem variable, no?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Resolved CSS linting issues. Just by running `yarn lint:css --fix`.

Issue seems fixed. Here are 4 screens illustration outline rings for hover and focus states, for both collapsed and expanded states.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Seems there is an extra setting allowing you to display as much levels in specific menu as you need.

For now closing the issue, but if you think that this behavior should be changed, feel free to re-open it

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I'm gonna mark it as Needs review since i've inserted few modifications

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Please test all 3 issues and review MR

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Pushed new commit with only fixing of the linting issues. I have tested MR and seems issue is fixed, now you can vertically scroll sub-nav bar

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

mmm, i can't reproduce it @rkoller

so you said:

enable automatic dark mode ticked, forced colors active and preer contrast set to more

In the system OS settings on Mac OS i have enabled dark mode
in MS Edge browser i have enabled forced-colors: active
and this preer contrast set to more i think you were talking about system OS settings again? I see range bar in the settings to increase contrast (or this setting somehow i can apply inside of MS Edge)?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Task is finished. Now Olivero, Claro and Umami are not overriding hover and focus styles of the navigation module

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

UPD:

Seems we can't go with @layer at-rules.

@layer is useful only when need to override user agent styles seems like. But any CSS property written by us (author styles) has bigger specificity than @layer's. Always. Which means if we will wrap some code in @layer - user themes will still override navigation module styles.

Will try to increase specificity of our selectors without @layer

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

After investigation i found that not only Umami overrides styles of Navigation module, but also Claro and even Olivero sometimes. It's related to some global selectors which have bigger specificity than our global:

:where(.admin-toolbar) *:focus {
    outline: var(--admin-toolbar-size-focus) solid var(--admin-toolbar-color-focus);
}

Here is screenshot illustrating that styles from Claro theme are overriding :focus on logotype in navigation bar.

And now i think that having such selectors in user themes is a pretty popular thing:

a:focus {
    ... something like setting outline globally for all links ...
}

And now we can't guarantee themes will never override navigation module styles. So we have to increase specificity of our selectors. To be sure we are safe in terms of styles no matter which theme is used.

@finnsky suggested to go with this way https://developer.mozilla.org/en-US/docs/Web/CSS/@layer and i will try to implement it now

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I also removed useful from my pov navigation_preprocess_breadcrumb hook which was added to add an extra link "Back to site" in breadcrumbs:

Tell me if we have to keep it or not. But from my pov it should be handled not in the scope of Navigation module

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I tried to reproduce in Safari with VoiceOver enabled - i was not able to do that. But i've got the idea that probably the fix provided in this MR https://www.drupal.org/project/navigation/issues/3438039 πŸ› Visual glitch while sub-menu is expanding / collapsing Needs review will resolve this problem. Let's try to re-reproduce again once side issue is fixed @rkoller

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Seems the fix was simple, ready for review

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I have resolved my feedback on MR, but once i have pushed new commit, we need again someone to review everything

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

mmm @ckrina i have pushed one little commit to get rid of `toolbar-button--level-1` selector.

But i also left a comment in MR. We have so many CSS variables for fonts which will never be used seems like. Tagging to `Needs work` for discussions

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Re #11
i think i made a mistake. Because seems on design 0-level links and buttons are equal to 1-level links and buttons in terms of text

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

1. Created list with font-size, line-height, font-weight and letter-spacing CSS variables.
2. Font-weight - because potentially someone will want to customize font. And we have 400, 500, 600, 700 font weights in our design system. Potentially custom font will never have some of those weights, so it will be easy to override font weights from css.
3. Letter spacings are also documented. Just to be able to customize it (coz it's pretty dangerous css property, which is working "fine" only with current font, but will not work with custom fonts)
4. Reworked tooltip a bit (now fully matching Figma)
5. Didn't touch breadcrumbs. We still have code related to breadcrumbs, but idk about the current state of this feature. I remember we did an override of breadcrumbs with some custom styles, but not sure if it's still actual feature or forgotten.

I suggest you to quickly review. In case of minor feedbacks -> let's create follow-up issue, because potentially we can get some (or lots of) conflicts.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

@ckrina just enable core theme "Stark" and set it as default theme. Then visit homepage )

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Rebased, conflicts resolved. Yarn lint:css tells that there are linting problems in toolbar-popover.pcss.css file. However i didn't touch this file in the scope of this ticket, so should be resolved in some other issues. Current MR contains only required changes by this ticket.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Kanchan Bhogade, seems you have to git pull from 1.x first. The errors says that there is not file NavigationTopBarTest.php to patch, however this file is exist in 1.x https://git.drupalcode.org/project/navigation/-/blob/1.x/tests/src/Funct...

Restoring needs review

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Can we get rid of ::before at all in toolbar buttons?
And to have just

... optional 2 first letters...

With the own tag and specific classname it would be much easier to deliver 2 letters. Also 100% it will be easier to integrate 3rd party icons.
Please share your opinion

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Also i see so many text variations on design. Are they all 100% used in Navigation module? Basically the scope only is sidebar + top bar. Did i miss something?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Would be nice to clarify this ticket. We don't have so granular components in Navigation module, like for instance component "Text" with 50 variations. I think this ticket is more about review of the current styles for the all texts?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Should be ok now. Works in MS Edge also

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

holy @ckrina. It works :O
gonna dig more & maybe complete this ticket

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I think better wait for https://www.drupal.org/project/navigation/issues/3427657 πŸ“Œ Implement the new designs to the drawer Needs review

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Here is screenshot from 10.3.0-dev

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Let's test it with drupal 11.x because related fix in Claro theme was provided only for 11.x https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/claro/...

For 10.x sticky table header doesn't work (due to missing fix in Claro).

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Actually let's have it under Active status. Since some work can be already done

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

It's better wait for https://www.drupal.org/project/navigation/issues/3427659 πŸ“Œ Implement drawer functionality Needs review first

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Let's wait for https://www.drupal.org/project/navigation/issues/3427659 πŸ“Œ Implement drawer functionality Needs review first, because MR out there contains lots of new colors which are hardcoded. Once task is fixed -> you can re-open this ticket

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Postponed because this functionality is already partially done here https://www.drupal.org/project/navigation/issues/3427659 πŸ“Œ Implement drawer functionality Needs review

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Another feedback is about uploaded file. Instead of filename "logo.svg" we'd better have preview?
And btw just got cool idea - is that for preview we can use the same box 32x32 pixels for the user so before saving the form he can already check how logotype will look in such small box.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

We also have to create new image style 32x32 with Scale effect (no upscale). Scale effect because we can't crop logo, and no upscale because we have static box 32x32 pixels. So logotype in this box with such image style effect will be always centered vertically and horizontally, which is ok.

But current text "Recommended image dimension 32 x 32 pixels" is not so meaningful. Because actually we don't care. Proportions of logotype can be any. We have to say to the user that logotype will be resized proportially without cropping and it will be displayed in 32x32 box and centered. Something like this, but shorter ;)

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

There are still several existing data() methods:

% yarn lint:core-js-passing

core/misc/ajax.js
  1104:17  error  Prefer WeakMap to $.data  jquery/no-data
  1111:31  error  Prefer WeakMap to $.data  jquery/no-data
  1580:7   error  Prefer WeakMap to $.data  jquery/no-data

core/misc/autocomplete.js
  224:13  error  Prefer WeakMap to $.data  jquery/no-data

core/misc/dialog/dialog.ajax.js
   59:31  error  Prefer WeakMap to $.data  jquery/no-data
   60:13  error  Prefer WeakMap to $.data  jquery/no-data
  105:24  error  Prefer WeakMap to $.data  jquery/no-data

core/misc/tabbingmanager.js
  297:25  error  Prefer WeakMap to $.data        jquery/no-data
  302:9   error  Prefer WeakMap to $.data        jquery/no-data
  314:25  error  Prefer WeakMap to $.data        jquery/no-data
  332:13  error  Prefer WeakMap to $.removeData  jquery/no-data
  340:13  error  Prefer WeakMap to $.data        jquery/no-data

βœ– 12 problems (12 errors, 0 warnings)

1. regarding ajax.js and 3 remaining errors - i think all 3 errors can be fixed, but need to research (i just don't have available time anymore for this, so skipped)

2. regarding autocomplete and dialog - there are some data methods which are still exist, which are related to the plugins itself (jquery autocomplete, jquery dialog). I didn't touch those 2 files at all, i think the better way is to wait for replacement of jquery autocomplete in core, and replacement of jquery dialog to native dialog element functionality.

3. regarding tabbingmanager - i didn't understand how to reproduce it in Drupal for making local tests. From what i saw in tabbingmanager.js - this file is fixable in terms of replacing jquery data() method

4. Also i prefer to use getAttribute() instead of dataset, coz getAttribute works seems at least 2x faster https://www.measurethat.net/Benchmarks/Show/14432/0/getattribute-vs-dataset

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

@Tirupati_Singh The displace functional described in core here https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/displace... so no matter what we are building - module or theme, we should respect core displace functionality.

This task have 2 problems:
1. Table header in Claro theme (see https://www.drupal.org/project/drupal/issues/3432298 πŸ› Sticky table header is not sticky if --drupal-displace-offset-top is not defined Fixed and btw this task is already fixed)
2. "data-offset-top" html attribute in Navigation module - was added always, but instead should be added when we are really have top bar rendered (this is what was fixed in 200 mr)

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Yes but resolving table problems should not be in the scope of navigation module

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I was thinking how to bring minimal effort for solving this idea and here it is:
1. We have to redo CSS logic, we need to get rid of ".toolbar-button::before" selector with mask-image and replace it by embedded svg image in twig template with source(), like:


// toolbar-button.html.twig

<button { ...props }>
  <span class="toolbar-button__icon">
    {{ source('path/to/root/assets/icons/folder/' ~ iconname, true) }} 
  </span>
</button>

btw, second param in source() twig function true required to prevent WSOD if template defined in first param is not found.

2. We put all the icons we have currently in assets/icons folder where filename of svg file = twig's iconname. So this is a strict rule, filename of icon matters.

3. In admin back office for admin menu links we can put some select field (or fieldset with the group of radio inputs). Each element = 1 icon. Preview of the icon + its label (based on icon filename). So user can browse all available icons at once and choose any icon he want.

4. Now how to use external icons? I'm not backender, but seems like we can create some folder in modules or themes, named "navigation-icons" where all custom svg icons can be stored. And then Drupal can scan all of the icons from specific folders and grab them and merge with the default icons stored in navigation module, so user in step 3 can browse all available icons at once.

Any opinions?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I have used new MR 6957, because 1251 have huge rebase with conflicts (i decided to not waste the time - that's only one reason).
Basically in my commit i was trying to resolve #17 πŸ“Œ Refactor (if feasible) use of jquery parseHTML function to use vanillaJS Needs work

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I this this task is related to this https://www.drupal.org/project/navigation/issues/3432173 πŸ“Œ Decide strategy to customize or provide 1st level menu items' icons Active where we need to define one global strategy how we can deliver own / custom icons.

1. background (mask)
2. iconic font
3. embed svg <- this is a winner by the way

And i think the initial approach with ::before pseudoselector directly on button itself (.toolbar-button) was a bit wrong. Since such kind of things (which should work with own icons, external icons and even 2 letters if no icon defined) requires own wrapper, like .

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Closed since fixed here https://www.drupal.org/project/navigation/issues/3423020 πŸ› Flickering when loading the page Needs review

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

gonna fix it now

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Confirmed described problem is really exist.

Because here https://git.drupalcode.org/project/navigation/-/blob/1.x/templates/top-b... we have "data-offset-top" attribute added no matter if top bar is rendering local_tasks or not.

And no matter if this block is hidden by some CSS rules or not -> core's displace() is still measuring height of this block https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/displace... and creating --drupal-displace-offset-top CSS property in style attribute of html tag. That's the reason why table header have top offset.

This issue requires 2 fixes:
1. first fix is already provided in MR - now we are not rendering "data-offset-top" attribute if no local_tasks in render.
2. and another fix should be done in core. I have created new ticket in core for Claro theme https://www.drupal.org/project/drupal/issues/3432298 πŸ› Sticky table header is not sticky if --drupal-displace-offset-top is not defined Fixed which i have fixed already . Reason is simple - once i have kicked "data-offset-top" attribute - there is no anymore css property "--drupal-displace-offset-top" defined, which breaks stickiness of table header (read description of core task i have created to understand better)

Production build 0.69.0 2024