Problem/Motivation

in nav-tabs.es6.js we have

    $(window)
      .on('resize.tabs', Drupal.debounce(toggleCollapsed, 150))
      .trigger('resize.tabs');

ToggleCollapsed should be an event listener to the media query defined inside.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3225621

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

nod_ created an issue. See original summary.

marcusvsouza’s picture

Assigned: Unassigned » marcusvsouza

Working on this.

marcusvsouza’s picture

Assigned: marcusvsouza » Unassigned
Status: Active » Needs review
droplet’s picture

Status: Needs review » Needs work

wrong upload? only re-formatting

nod_’s picture

Thanks for the patch, although the goal is to remove the resize event listener and replace it with an event listener on a media quary, not remove the jQuery event listener

marcusvsouza’s picture

Ok, I misunderstood the issue description, i'll work on the changes.Thanks for the heads up!

sagarchauhan’s picture

Status: Needs work » Needs review
StatusFileSize
new2.51 KB

Created the patch with refactored code as per the description.

marcusvsouza’s picture

The changes in the patch #8 are acquired with the instructions on the comment #6 , I commited the changes for more easier review by the maintainers.

marcusvsouza’s picture

marcusvsouza’s picture

Status: Needs review » Reviewed & tested by the community

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

nod_’s picture

Status: Reviewed & tested by the community » Needs review

few nitpick.

gabrielda’s picture

Assigned: Unassigned » gabrielda

Hello I will review this one.

gabrielda’s picture

Status: Needs review » Needs work

Doing the changes appointed in comment #13.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gabrielda’s picture

Assigned: gabrielda » Unassigned

I couldn't progress much, hence I am unassigning myself.

ckrina’s picture

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

kostyashupenko’s picture

Status: Needs work » Needs review

Rebased & feedbacks resolved

Johnny Santos’s picture

Assigned: Unassigned » Johnny Santos

I will be working on this review

Johnny Santos’s picture

Assigned: Johnny Santos » Unassigned
Status: Needs review » Needs work

I have tried to download the code with a fresh drupal 9 install,then I followed using fetch and switching the branch, but unfortunately couldnt make it work.
It returns to me a white screen, even tried to find whats the matter with drush watchdog and it also didnt worked somehow ( OCI runtime exec failed: exec failed: container_linux.go:380: starting container process caused: exec: "drush": executable file not found in $PATH: unknown)
Switching back to "9.4" branch it starts working again.

andy-blum’s picture

@marcusvsouza can you re-target this MR to the 9.4.x branch?

marcusvsouza’s picture

Done!

Rodrigo Borges’s picture

Assigned: Unassigned » Rodrigo Borges

Working on it!

andy-blum’s picture

Status: Needs work » Needs review

@Rodrigo, the 9.4.x & 10.0.x branches are both ready for review

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Branches 9.4.x and 10.0.x are good to go, thanks!

chi’s picture

Use media query event listener instead of a listener on the resize event

I do not understand how the media query event can replace the resize event. It only fires when the result of defined query changes. In this case it happens on 48rem breakpoint. The script is supposed to collapse the tabs when they don't fit the container width. For this purpose we need to track all resizes. I think the right tool for this is ResizeObserver.

nod_’s picture

In theory yes, but we don't do a container query type of thing here. The resize callback just checks for the mediaquery result, so binding an even to the media query itself does exactly the same in a more performant way.

chi’s picture

The resize callback just checks for the mediaquery result

@node_ The resize callback is supposed to change tabs shape to vertical or horizontal depending on whether or not they fit the container.
With the current approach the tabs will stick in their initial shape when the container size is larger than 48rem.

lauriii’s picture

Should this issue be against 10.0.x given that it seems like this isn't supported by IE 11: https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList/change_e... 🤔

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Moving to needs review to get an answer for #33

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new150 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sagarchauhan’s picture

Status: Needs work » Needs review
Issue tags: -JavaScript +JavaScript

Rerolled the patch and raised MR against 11.x

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

MR 2013 even though pointing to 10.x still applied fine for 11.x

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Asked question in the MR

lauriii’s picture

Assigned: Rodrigo Borges » Unassigned
smustgrave’s picture

Status: Needs review » Needs work

For the one open thread opened by lauriii

sagarchauhan’s picture

Status: Needs work » Needs review

Addressed the issue raised by lauriii in #42

alextran changed the visibility of the branch 3225621-11.x to hidden.

alextran changed the visibility of the branch 3225621-10.0.x to hidden.

alextran changed the visibility of the branch 3225621-11.x to active.

alextran changed the visibility of the branch 3225621-10.0.x to active.

adelansari’s picture

Assigned: Unassigned » adelansari
Status: Needs review » Reviewed & tested by the community

Reviewed the changes in the code and tested the responsiveness in the following url by resizing the page:
http://drupal.ddev.site/admin/structure/types/manage/article/fields

lauriii changed the visibility of the branch 3225621-10.0.x to hidden.

lauriii changed the visibility of the branch 3225621-11.x to hidden.

lauriii changed the visibility of the branch 3225621-9.4.x to hidden.

lauriii changed the visibility of the branch 3225621-use-media-query to hidden.

  • lauriii committed 0466b818 on 11.x
    Issue #3225621 by sagarchauhan, andy-blum, marcusvsouza, kostyashupenko...

  • lauriii committed 29af2344 on 10.3.x
    Issue #3225621 by sagarchauhan, andy-blum, marcusvsouza, kostyashupenko...
lauriii’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 0466b81 and pushed to 11.x. Also cherry-picked to 10.3.x. Thanks!

  • lauriii committed 0466b818 on 11.0.x
    Issue #3225621 by sagarchauhan, andy-blum, marcusvsouza, kostyashupenko...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.