Remote: https://github.com/paulirish/matchMedia.js

matchMedia is written with debounce feature now.

- reduce dependencies & added "matchmedia.addListener" ( sometimes, it only need matchMedia )

CommentFileSizeAuthor
#42 interdiff-39-42.txt523 bytestarekdj
#42 update_matchmedia-2207629-42.patch7.96 KBtarekdj
#39 update_matchmedia-2207629-39.patch7.89 KBtarekdj
#37 interdiff-33-37.txt7.5 KBtarekdj
#37 update_matchmedia-2207629-37.patch7.94 KBtarekdj
#33 2207629-33.patch11.1 KBdevin carlson
#33 interdiff-2207629-29-33.txt836 bytesdevin carlson
#29 2207629-29.patch11.06 KBrpayanm
#16 2207629-11-matchmediaJS.patch10.93 KBdroplet
#10 interdiff.txt1.18 KBdroplet
#10 2207629-10-matchmediaJS.patch11.13 KBdroplet
#9 interdiff.txt1.18 KBdroplet
#9 2207629-9-matchmediaJS.patch6.12 KBdroplet
#5 matchMedia.patch11.42 KBdroplet
#2 matchMedia.patch1.69 KBdroplet
matchMedia.patch1.67 KBdroplet

Comments

Status: Needs review » Needs work

The last submitted patch, matchMedia.patch, failed testing.

droplet’s picture

droplet commented
StatusFileSize
new matchMedia.patch1.69 KB

Ouch, forget generate the new patch after testing. Here is new patch with correct path.

droplet’s picture

droplet commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: matchMedia.patch, failed testing.

droplet’s picture

droplet commented
Status: Needs work » Needs review
StatusFileSize
new matchMedia.patch11.42 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This proposal seems to conflict with the latest proposals in #1848500: Use performance optimized matchmedia polyfill

droplet’s picture

droplet commented

Both script are very similar. I could see the author name: "WebLinc, David Knight." there. This patch version is rewritten by him in upstream.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ b/core/core.libraries.yml
@@ -649,13 +649,19 @@ jquery.ui.widget:
+  # @todo Stable release required for Drupal 8.0.
   version: VERSION
...
+  # @todo Stable release required for Drupal 8.0.
+  version: VERSION

Can you clarify (in code) which version we're updating to?

In case this is the upcoming/not-yet-released 0.1.1, then it should state:

version: master
commit: thehashyoudownloaded
droplet’s picture

droplet commented
Status: Needs work » Needs review
StatusFileSize
new 2207629-9-matchmediaJS.patch6.12 KB
new interdiff.txt1.18 KB
droplet’s picture

droplet commented
StatusFileSize
new 2207629-10-matchmediaJS.patch11.13 KB
new interdiff.txt1.18 KB

Add back the commit hash

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

The issue summary claims

"matchmedia.addListener" ( sometimes, it only need matchMedia )

...but I'm not able to see a proof for that in the actual patch?

If there is no use-case for the split in core, then let's merge the two library declarations into a single.

The last submitted patch, 9: 2207629-9-matchmediaJS.patch, failed testing.

droplet’s picture

droplet commented

@sun,

Checkout CORE Seven theme.

nav-tabs.js

var notSmartPhone = window.matchMedia('(min-width: 300px)');

it used resize event:
$(window).on('resize.tabs', Drupal.debounce(handleResize)).trigger('resize.tabs');

(resize based on tab sizes / content width, not the viewport size)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Update JS lib: matchMedia » Update matchMedia library to latest release
Assigned: Unassigned » nod_
Status: Needs review » Needs work

@nod_: Can you confirm @droplet's statements? I'm not familiar with those custom event listeners.

A new 0.2.0 release was created a few days ago:

https://github.com/paulirish/matchMedia.js/releases

This means that we should pull in exactly the tagged code + remove the @todo about creating a new upstream release from the library declaration (replacing version with the actual version, and removing 'commit').

The only change since 0.2.0 appears to be an oversight in bower.json, which is irrelevant for us (at this point):
https://github.com/paulirish/matchMedia.js/compare/0.2.0...master

@droplet: Can you perform these changes?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Normal » Major
droplet’s picture

droplet commented
Status: Needs work » Needs review
StatusFileSize
new 2207629-11-matchmediaJS.patch10.93 KB

Status: Needs review » Needs work

The last submitted patch, 16: 2207629-11-matchmediaJS.patch, failed testing.

droplet’s picture

droplet commented
Status: Needs work » Needs review

16: 2207629-11-matchmediaJS.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 16: 2207629-11-matchmediaJS.patch, failed testing.

droplet’s picture

droplet commented
Status: Needs work » Needs review

16: 2207629-11-matchmediaJS.patch queued for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

16: 2207629-11-matchmediaJS.patch queued for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented

16: 2207629-11-matchmediaJS.patch queued for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Assigned: nod_ » Unassigned

Looks ok to me, I'll wait for the testbot before gooing further.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

All good :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Could we not put a minified version in instead?

xjm’s picture

xjm
she/her
Primary language English
commented

@catch, wouldn't that be in scope for #1341792: [meta] Ship minified versions of external JavaScript libraries?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch in #16 needs a reroll.

catch’s picture

catch
he/him
Primary language English
commented

@xjm I think we might as well use the minified versions when doing updates at this point.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2207629-29.patch11.06 KB

rerolling...

Status: Needs review » Needs work

The last submitted patch, 29: 2207629-29.patch, failed testing.

Status: Needs work » Needs review

rpayanm queued 29: 2207629-29.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 29: 2207629-29.patch, failed testing.

devin carlson’s picture

devin carlson commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2207629-29-33.txt836 bytes
new 2207629-33.patch11.1 KB

This should fix the test failures.

I'm not sure what to do about minified versions of the files, since they aren't included with the repository.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Patch looks good. I'd say that to ship with a minified version, we want to apply uglifyjs2 to the source. NW only for that, if catch hadn't asked to minify this, this would be RTBC.

droplet’s picture

droplet commented

Here're the online version of 2 best minifier:
http://lisperator.net/uglifyjs/#demo
http://closure-compiler.appspot.com/home

+++ b/core/core.libraries.yml
@@ -752,17 +752,21 @@ jquery.ui.widget:
+  version: &matchmedia_version 0.2.0
+  license: &matchmedia_license
...
+  version: *matchmedia_version
+  license: *matchmedia_license

incorrect ?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#35: No, that's correct. See core.libraries.yml, the entries for jQuery UI, for similar examples.

tarekdj’s picture

tarekdj commented
Status: Needs work » Needs review
StatusFileSize
new update_matchmedia-2207629-37.patch7.94 KB
new interdiff-33-37.txt7.5 KB

Minified using uglifyjs2 :

$ uglifyjs matchMedia.js -m -o matchMedia-min.js
$ uglifyjs matchMedia.addListener.js -m -o matchMedia.addListener-min.js
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Thank you! Two nitpicks, after which this will be RTBC:

  1. --- /dev/null
+++ b/core/assets/vendor/matchMedia/matchMedia-min.js

--- /dev/null
+++ b/core/assets/vendor/matchMedia/matchMedia.addListener-min.js

    All minified JS in core uses .min.js, not -min.js. Let's be consistent with that. 

  2. +++ b/core/assets/vendor/matchMedia/matchMedia-min.js
@@ -0,0 +1 @@
\ No newline at end of file

+++ b/core/assets/vendor/matchMedia/matchMedia.addListener-min.js
@@ -0,0 +1 @@
\ No newline at end of file

    Please add newlines at the end of these files.

tarekdj’s picture

tarekdj commented
Status: Needs work » Needs review
StatusFileSize
new update_matchmedia-2207629-39.patch7.89 KB

updated patch.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Should the license link to either https://github.com/paulirish/matchMedia.js/blob/0.2.0/LICENSE.txt (current patch keeps it at 0.1.0)?

tarekdj’s picture

tarekdj commented
StatusFileSize
new update_matchmedia-2207629-42.patch7.96 KB
new interdiff-39-42.txt523 bytes

@catch you're right!
Fixed the license.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Ugh, missed that. Good catch, catch.

The last submitted patch, 37: update_matchmedia-2207629-37.patch, failed testing.

The last submitted patch, 39: update_matchmedia-2207629-39.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical
Status: Reviewed & tested by the community » Fixed

Retrospectively marking critical as a child of #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0.

Committed/pushed to 8.0.x, thanks!

  • catch committed 37e7d70 on 8.0.x 
    Issue #2207629 by droplet, tarekdj, Devin Carlson, rpayanm: Update...

Status: Fixed » Closed (fixed)

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