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
| Comment | File | Size | Author |
|---|---|---|---|
| #37 | 3225621-nr-bot.txt | 150 bytes | needs-review-queue-bot |
| #8 | 3225621-8.patch | 2.51 KB | sagarchauhan |
Issue fork drupal-3225621
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
Comment #2
marcusvsouza commentedWorking on this.
Comment #4
marcusvsouza commentedComment #5
droplet commentedwrong upload? only re-formatting
Comment #6
nod_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
Comment #7
marcusvsouza commentedOk, I misunderstood the issue description, i'll work on the changes.Thanks for the heads up!
Comment #8
sagarchauhan commentedCreated the patch with refactored code as per the description.
Comment #9
marcusvsouza commentedThe 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.
Comment #10
marcusvsouza commentedComment #11
marcusvsouza commentedComment #13
nod_few nitpick.
Comment #14
gabrieldaHello I will review this one.
Comment #15
gabrieldaDoing the changes appointed in comment #13.
Comment #17
gabrieldaI couldn't progress much, hence I am unassigning myself.
Comment #18
ckrinaComment #20
kostyashupenkoRebased & feedbacks resolved
Comment #21
Johnny Santos commentedI will be working on this review
Comment #22
Johnny Santos commentedI 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.
Comment #23
andy-blum@marcusvsouza can you re-target this MR to the 9.4.x branch?
Comment #24
marcusvsouza commentedDone!
Comment #27
Rodrigo Borges commentedWorking on it!
Comment #28
andy-blum@Rodrigo, the 9.4.x & 10.0.x branches are both ready for review
Comment #29
nod_Branches 9.4.x and 10.0.x are good to go, thanks!
Comment #30
chi commentedI 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.
Comment #31
nod_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.
Comment #32
chi commented@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.
Comment #33
lauriiiShould 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... 🤔
Comment #34
lauriiiMoving to needs review to get an answer for #33
Comment #37
needs-review-queue-bot commentedThe 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.
Comment #40
sagarchauhan commentedRerolled the patch and raised MR against 11.x
Comment #41
smustgrave commentedMR 2013 even though pointing to 10.x still applied fine for 11.x
Comment #42
lauriiiAsked question in the MR
Comment #43
lauriiiComment #44
smustgrave commentedFor the one open thread opened by lauriii
Comment #45
sagarchauhan commentedAddressed the issue raised by lauriii in #42
Comment #51
adelansari commentedReviewed 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
Comment #58
lauriiiCommitted 0466b81 and pushed to 11.x. Also cherry-picked to 10.3.x. Thanks!