πŸ‡·πŸ‡ΊRussia @kostyashupenko

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

Merge Requests

More

Recent comments

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Rebased, conflicts were resolved carefully. However it needs review again.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Any other opinions how to solve this task with minimal effort?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Added several feedbacks. However MR needs huge work still

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Wondering what is the cause of that fact that once() in that case doesn't actually work (works 2 times)

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Long task.
Forced colors active disables background images and mask images.
I see only one solution -> is to rework method of rendering icons in our navigation. Instead of CSS background images we should use {{ source(path-to-icon, true) }} and rework css accordingly

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Re #5
i was thinking what i can do - i decided to switch logic a bit in CSS. Now `font-family: inherit` is still placed in two difference places, but that's why:
1. I want to keep such "base" styles only in the scope of navigation module
2. Top bar is placed in the difference place in DOM tree than admin toolbar.
Looks ok now i think

Re #6
This issue can be reproduced only with some custom themes, not with core themes like Claro or like Olivero (the theme on your screenshot). These two themes have `button { font-family: inherit }` already.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

It can be a follow-up. I just checked - navigation becomes visible once i'm granting permissions to "Author" role users in Umami.

Added core task (postponed status).

Restoring RTBC here

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

MR rebased

Seems submit is working fine in top bar. But i'm thinking that maybe we shouldn't move original submit, but instead - duplicate? I find it unclear when i'm scrolling down the page and there is no "Save" button in the bottom.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Color module is no longer in core. Should we close this issue?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

To test replace
"lint:core-js-passing": "node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json .",
by
"lint:core-js-passing": "node ./node_modules/eslint/bin/eslint.js --config=.eslintrc.passing.json .",

and run yarn lint:core-js-passing

πŸ‡·πŸ‡Ί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

All javascript libraries which helps to scroll-lock across devices - it's not about `overflow` CSS property only.
`overflow: hidden` is still not working on iOS devices (probably even desktop Safari).

Moveover it will disable `position: sticky` for the child elements somewhere in tree.

So this task & MR requires deep tests

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Just my thoughts.
1. Each form can have several submit buttons, i.e.: Save, Preview, Something else? Which means we can't just use `for` html attribute with form_id attribute value.
2. Instead we could use somekind of

  <label for="edit-submit" tabindex="0">Preview</label>
  <label for="edit-submit--2" tabindex="0">Save</label>
  <label for="edit-submit--3" tabindex="0">Something else?</label>

The solution above is already working, but i don't think it's too much accessible honestly, correct me if i'm wrong.

3. Non else "native" solutions are working.
4. Potentially we could use javascript for this. For everything: creation of the "fake" submit buttons in top bar and for binding listeners.

Few more thoughts but about other thing:
1. I think this task should be not only about moving "Save" button in top bar, but instead we should move all actions of the form in top bar.
2. On which pages we should do this trick? In Drupal there are many pages in admin back office with lots of forms, which has actions. I bet for any admin page where we have form, isn't it ?
3. Potentially one admin page can contain two or more forms (but i can not be sure here) - obviously we shouldn't move all actions from all forms in top bar. What to do in that case?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I did the easiest available thing. Since expand side nav button is a part of top_bar template - i decided to keep the same logic.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Thanks @larowlan finally all green

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I would move `toolbar-block` BEM block on top of drupal's block. Again - we are rendering blocks with menus inside. Blocks can render label and content. So i'm thinking if `toolbar-block` classname should be added to `navigation-block.html.twig` as a wrapper. For menus inside - can be just `toolbar-block__menu` or `toolbar-block__section`.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

pushed something. Cspell failure appeared after rebase, not sure how to fix it

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Trying to get more backend skills. Let me try to handle it

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

CI reports in spellcheck job is not so informative

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Seems rebase didn't help. After local investigation also no results. No errors on spellcheck. Can you guide me if you will have an ability to fix it - what was a cause of it?

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

New changes are based on BEM methodology aswell (sorry for wrong status)

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I'm not backender :) but seems MR helped

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Thanks, MR updated, very detailed answer written. Please check

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Maybe i will say dumb thing, but what if we will not use UI provided by contextual core module, and instead we will have "Edit Navigation Bar" just regular link (same link as menu links in our navigation), with icon. Like this:

I'm suggesting it because:
1. It can be a static link which is always visible / pretty clear to the user. It's in the style of navigation design. Shown no matter if navigation block is hovered or not.
2. Works perfect already (thx to already written js) in collapsed state
3. Initial behavior of contextual links is to show circle with pencil icon only when container is hovered. Which points me to the though - we will need to rewrite javascript of contextual module to adapt our needs for collapsed navigation behavior.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Just figured out new problems. Regarding position on expanded navigation - clear. We also can make it non-sticky on scroll. Works fine locally (but i didn't push anything for now).

However here is the issue in collapsed state.
Here is default position i tried to implement (equal spaces above contextual circle and below):

But when i'm hovering first menu link - it's a bit overlapped by contextual link:

But when i'm trying to browse contextual menu, a big part of contextual menu goes out of viewport:

Obviously i tried to display contextual menu bar in collapsed navigation from left side till right (by default it grows from the right side to the left, that's why it's trimmed on my previous screenshot). However it didn't help:

Because parent divs has overflow: auto; - which is totally fine, navigation menu is vertical scrollable.
So all these attempts are really noising to me and i wasn't able to come up with something better. I also was thinking with `position: fixed` for contextual links in collapsed variant -> but in that case it will be kind of "sticky" on scroll, because position fixed will overlap everything. So not sure where exactly i have to put contextual links in collapsed variant.

Also no issues at all in expanded variant.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Fixed template a bit. Now you can see contextual links. However it requires design.
Current behavior is:

1. Contextual links are sticky on scroll - not sure if we need it

2. Contextual links can overlap "close" icon for

3. Contextual links in collapsed variant can overlap logotype

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I'm gonna remove Book from umami profile and from core themes here https://www.drupal.org/project/drupal/issues/3410220 πŸ“Œ Remove book module from all core themes Postponed

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Gonna remove book from umami profile and from themes.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Added proof of concept in merge request. You can test on '/admin/config/development/settings' page

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Regarding this issue her is a list of states which can really damage layout shifting:

  • visible
  • invisible
  • expanded
  • collapsed

First two states can be used for any thing, while the last two states are for the details html element (playing around open html attribute).

Now about visible/invisible states. We can have something like

   $form['something'] = [
      '#type' => 'fieldset',
      '#title' => $this->t('Some title'),
      '#states' => [
        'visible' => $something,
      ],
    ];

With the code above we adding state `visible` directly to the container of some elements.
While with this code:

  $form['something'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Something'),
      '#states' => [
        'visible' => $something,
      ],
    ];

we adding state directly to checkbox. But rendering system in Drupal provides form_element hook theme which is actually a wrapper of form elements. And according to states.js

  $document.on('state:visible', (e) => {
    if (e.trigger) {
      let $element = $(e.target).closest(
        '.js-form-item, .js-form-submit, .js-form-wrapper',
      );
      // For links, update the state of itself instead of the wrapper.
      if (e.target.tagName === 'A') {
        $element = $(e.target);
      }
      $element.toggle(e.value);
    }
  });

we are not actually hiding/showing checkbox itself. We hiding/showing container (form-element container) element.

=====

What i want to say is that we can't solve this issue just by using CSS media query "scripting". Instead, here should be more complex logic which can give frontender some attributes or classnames of initial state.

Here is example:
1. Visit /admin/config/development/settings page
2. Initially all checkboxes are unchecked.
3. If you will toggle "Twig development mode" checkbox and then click on save btn
4. On the next visits of this page initial state will be different. Because checkbox is initially checked, so container element which is sensitive to the state of this checkbox should be initially visible. <-- here we are missing some helpful attributes or classnames, just to detect initial state to understand which CSS rules we should write.

If initial state should be "visible" -> we don't need to hide anything.
If initial state should be "hidden" -> we should hide element using CSS.

====
Summarizing:
We have to have some extra html attribute or classname above the element with state which explains initial behavior.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Regarding my message #13 πŸ› Tabledrag creates jank (layout shift) on page load Needs work i see dropbutton links already were fixed & closed in https://www.drupal.org/project/drupal/issues/3361315 πŸ› Dropbutton quickly shows/hides its menu on pageload causing layout shift Fixed

Here is my attempt to fix vertical layout shifting and a workaround of "Show row weights" functionality. Previously this button (Show row weights) was added from javascript which creates visual layout shifting (see before/after patch videos below). I decided to change logic a bit, see merge request.

Before patch:

After patch:

Changing status of the ticket to "Needs review" just to bump this issue & discuss this task more carefully. However the fix i provided is not enough to close this ticket, since the biggest layout shifting creates "Weight" column (see explanation #13 πŸ› Tabledrag creates jank (layout shift) on page load Needs work )

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Also current MR https://git.drupalcode.org/project/drupal/-/merge_requests/5782 doesn't doing anything, since className `.tabledrag-hide` appears from JS. Moreover we can't just use `display: none;` here, because if "Show row weights" button was pressed -> Weight column should be visible by default.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Kind of tricky task, which requires much more than just scripting media query.
Here is a snippet of only the problem i was able to solve

@media (scripting: enabled) {
  .draggable-table:not([data-once]) {
    margin-top: calc(28rem / 16 + var(--space-l));
  }
}

The snippet above is for selector .tabledrag-toggle-weight-wrapper only. Once this element is not created yet by js (but with the help of CSS class draggable-table added to the table we already know our table will be draggable -> we can prevent vertical layout shifting.

===========

However for the rest of things:
1. like table column "Weight" which can be hidden/visible based on localStorage
2. also dropbutton links which are expanded by default (while no js)
3. Dragging icon on the left side of each draggable row
-> this is all creates a lot of layout shifting and unfortunately we can not solve these 3 issues just by CSS media scripting query.

Here is more detailed answer.
Regarding column "Weight" - in the initial DOM render we don't know which column exactly should have visible/hide functionality. This is all done by javascript logic and also it is based on localStorage. For example if "Show row weights" button was clicked and then you will refresh the page -> Weight column will be shown by default (based on localStorage). So from CSS we are not able to get the states from localStorage (obviosly) and we don't have selectors we could use for.

Regarding dragging icon on the left side -> same situation. When "Weight" column is shown - we don't have Dragging icon anymore (coz ordering should be handeled by Weight selectbox) and this logic is done on js side and again based on localStorage.

Regarding dropbutton links <- one of the problems we can solve, but it smells like another task where we need to fix first dropbutton links only.

πŸ‡·πŸ‡Ί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

Same question to `navigation-block.admin.js` file. It's almost identical to core's `block.admin.js`.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Here is a new MR https://git.drupalcode.org/project/navigation/-/merge_requests/161
which contains manual cherry-pick of #2
+ some other fixes, now ready for code-review

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

i have a question regarding this file https://git.drupalcode.org/project/navigation/-/blob/1.x/js/navigation-b... in terms of this task

This js file is almost identical to https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...
Is there a real reason regarding duplication of file? Can we just provide similar HTML markup than block.js expecting? By providing all the necessary classnames and table ID (maybe some other ID, dunno)

If we are not going to fully depend on block.js, maybe we can partially depend on block.js and instead of overwriting a whole file -> we can override only necessary methods / full behaviors.

And finally - this file contains some jquery prototyping, like $.fn.drupalSetSummary , and this kind of things is defined in https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/form.js?...
It's written on jQuery, so makes sense to nest it from core and don't override on the own javascript

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Kind of old problem (since 2017) which is quite important and still not fixed https://www.drupal.org/project/drupal/issues/2856572#comment-15395620 ✨ Views Pager includes duplicates with Random Sort Needs work

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Bump. This is still an issue in Drupal 10.2

I just have generated several nodes of some content type -> then I created new view with random sorting (no matter if Ajax enabled or not).
Look at `16` number on the gif above, it's often appears on several pages.

Moreover, here is another use case.
Let's say we have some taxonomy terms of some vocabulary. And some terms has same weight. And guess a view which is displaying those terms with the sorting only by weight (no matter asc or desc and no matter if ajax enabled). In the end some of the terms will be duplicated on different pages.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Regarding that fact that overflow != visible removes position sticky behavior - it's clear. Just need to understand how exactly it can be reproduced (when table can live in the element with overflow hidden for example)

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I can't figure out about which tabs and overflow you talking. Just did everything:

- install webform module
- enable webform and webform_templates submodule
- go to /admin/structure/webform/templates
- scroll down and check sticky header

and table header is sticky on scroll

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Can't guess a case when `document.URL === link.href` can be failed
Even if we have `?` queries

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

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

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Please try again. I wasn't able to get any errors. There was a tiny rebase with no conflicts, however patch could be applied, so idk.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Added logic, and also added for the next section "Choose an option below" coz it also have grid-style radio blocks.

Logic in javascript is sensitive to the amount of grid columns.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Grabbed. Tomorrow morning result

πŸ‡·πŸ‡Ί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

+1 for Vanya
No words, just screenshot from my d.org profile !

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

The fastest option was to create new MR instead of doing hard rebases.
MR 151 opened, and i moved manually all the changes from the previous MR 141. Previous MR worked perfectly and i wanted to merge it and change status to "Fixed", but since hard rebase / recreation of the new MR -> this issue needs review again

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

kostyashupenko β†’ changed the visibility of the branch 3402899-one-menu-opening to hidden.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

kostyashupenko β†’ changed the visibility of the branch close-all-when-one-opened-3402899 to hidden.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

We can't use scrollbar-gutter as it is, because:
1. It's good to have it only when scrollbar is always exist.
2. If scrollbar is not exist -> see gif. On this gif i'm just checking/unchecking scrollbar-gutter property. On different operating systems / computers / specific appearance preferences on the system, scrollbar can behaves differently. So sometimes when scrollbar is hidden at the moment, this CSS property adds big end gap, which from my pov breaks design.

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

Fixed for ajax case. Also rebased

πŸ‡·πŸ‡ΊRussia kostyashupenko Omsk

I found 2 issues:
1. I forgot to put one more CSS line (now fixed)
2. on your video you have enabled aggregration (did you clear cache?). Even with previous commit you should have width: 100% on the .claro-autocomplete CSS selector, but i don't see it in your video.

Anyway let's re-test again, now works even with opened navigation. Sorry )

Production build https://api.contrib.social 0.61.6-2-g546bc20