Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
138213007:
Fixes for two JS bugs on DCC (Closed)
|
Created:
6 years, 11 months ago by Arthur Evans Modified:
6 years, 10 months ago Reviewers:
ericbidelman, mkearney1, Jeffrey Yasskin, Renato Mangini (chromium), not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFrom Eric, fixes for two bugs:
* Expand/collapse doesn't work on mobile if the page is loaded in landscape mode where
width > 580px, then rotated into portrait where width < 580px.
* Fatnav should only close on click, not on mouseleave.
To test expand/collapse bug, open link in standard browser and squeeze window down to mobile width (<580px). Without the fix, this results in the sections collapsing, but not expanding when clicked. Reloading the page at the smaller width restores expand/collapse behavior.
With the fix, the expand/collapse should work correctly regardless after resizing the browser window. (This can also be tested using a mobile device of the appropriate size, such as a Nexus 4. Resizing the desktop window is a bit easier).
A sample page with collapsible sections is:
multidevice/data-compression.html
BUG=
R=kalman@chromium.org
Patch Set 1 #
Total comments: 17
Patch Set 2 : Further fixes based on feedback. #Patch Set 3 : More fixes #
Messages
Total messages: 17 (0 generated)
Hi Ben, Can you review these JS fixes from Eric? They seem to work fine on my machine.
Renato or Eric - could you take ownership of the docs UI now? I'll let [one of] you review this.
lgtm
On 2014/01/22 18:47:30, kalman wrote: > Renato or Eric - could you take ownership of the docs UI now? I'll let [one of] > you review this. I don't think either Eric or Renato is in the OWNERS file... Do we need owner approval? Or do I just need to check the "Commit" button?
lgtm
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2014/01/22 22:09:00, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a lowly provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. OK, apparently we need a review from a committer...
Hi Jeffrey, This is a UI fix for the doc server that needs review by a committer. Can you take a look at it? Thanks, Arthur
https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/js/article.js (right): https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:14: isLargerThanMobileQuery.addListener(onMediaQuery); I'm surprised this works before onMediaQuery is defined. Maybe I just don't understand JS evaluation order. Could you put this .addListener next to the eager call, so all the related logic is together? https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:17: var sidebaroOffsetTop = null; spelling: "sidebaro"? https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:21: // Bomb out if we're not on an article page and have a TOC. s/if we're not/unless we're/ to avoid precedence problems. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:42: function onScroll(e) { We'll be able to replace this with position:sticky as soon as that comes out from behind a flag: http://www.chromestatus.com/features/6190250464378880. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:58: document.body.classList.add('no-permalink'); Surely this can be done in the CSS, inside a media query, instead of in javascript. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:64: // Toggle collapsible sections (mobile) This is getting registered in more places than just mobile. Also, please end comments with a period so it's clear you're done with your thought. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:87: Try not to add extra empty lines at the end of the file.
@ericbidelman, do you want me to address these comments? On 2014/02/04 22:22:01, Jeffrey Yasskin wrote: > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > File chrome/common/extensions/docs/static/js/article.js (right): > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:14: > isLargerThanMobileQuery.addListener(onMediaQuery); > I'm surprised this works before onMediaQuery is defined. Maybe I just don't > understand JS evaluation order. > > Could you put this .addListener next to the eager call, so all the related logic > is together? > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:17: var sidebaroOffsetTop = > null; > spelling: "sidebaro"? > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:21: // Bomb out if we're not > on an article page and have a TOC. > s/if we're not/unless we're/ to avoid precedence problems. > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:42: function onScroll(e) { > We'll be able to replace this with position:sticky as soon as that comes out > from behind a flag: http://www.chromestatus.com/features/6190250464378880. > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:58: > document.body.classList.add('no-permalink'); > Surely this can be done in the CSS, inside a media query, instead of in > javascript. > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:64: // Toggle collapsible > sections (mobile) > This is getting registered in more places than just mobile. > > Also, please end comments with a period so it's clear you're done with your > thought. > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:87: > Try not to add extra empty lines at the end of the file.
Renato, that would be great. Thanks. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/js/article.js (right): https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:58: document.body.classList.add('no-permalink'); It can, but .no-permalink is a generic class we're using throughout the site and which affects other related children. On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > Surely this can be done in the CSS, inside a media query, instead of in > javascript.
https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/js/article.js (right): https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:58: document.body.classList.add('no-permalink'); Can you show me where? https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium$&q... returns only 4 results, all of which look like they'd be covered by this one media rule. On 2014/02/04 22:53:42, ericbidelman wrote: > It can, but .no-permalink is a generic class we're using throughout the site and > which affects other related children. > > On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > > Surely this can be done in the CSS, inside a media query, instead of in > > javascript. >
https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/js/article.js (right): https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:58: document.body.classList.add('no-permalink'); Yep, https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext... is the one. I'd like to stick with this single toggle since it's already in place. Introducing an additional media query seems overkill, especially since this query is only evaluated once 99% of the time. On 2014/02/04 22:58:42, Jeffrey Yasskin wrote: > Can you show me where? > https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium%24... > returns only 4 results, all of which look like they'd be covered by this one > media rule. > > On 2014/02/04 22:53:42, ericbidelman wrote: > > It can, but .no-permalink is a generic class we're using throughout the site > and > > which affects other related children. > > > > On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > > > Surely this can be done in the CSS, inside a media query, instead of in > > > javascript. > > >
I've updated the comments and moved the MQ event listener to the end of the file. Jeffrey, can you LGTM or reply to the two remaining points -- Eric commented on the .no-permalink class and I commented on the click event listener on 'h2's -- not sure if we need to make any changes here. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/js/article.js (right): https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:14: isLargerThanMobileQuery.addListener(onMediaQuery); On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > I'm surprised this works before onMediaQuery is defined. Maybe I just don't > understand JS evaluation order. > > Could you put this .addListener next to the eager call, so all the related logic > is together? Moved query definition and listener to the end. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:17: var sidebaroOffsetTop = null; On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > spelling: "sidebaro"? Done. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:21: // Bomb out if we're not on an article page and have a TOC. On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > s/if we're not/unless we're/ to avoid precedence problems. Done. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:64: // Toggle collapsible sections (mobile) On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > This is getting registered in more places than just mobile. > > Also, please end comments with a period so it's clear you're done with your > thought. Since the "mobile" logic is based purely on width, some devices will flip back and forth between the mobile & non-mobile views when orientation changes. Should we unregister and re-register the click listener in the media query listener? https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:87: On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > Try not to add extra empty lines at the end of the file. Done.
Patch set 2 is a diff against patch set 1 rather than a diff against the Chrome tree, so you won't be able to commit it. Be sure the branch you're running `git cl upload` from has its 'upstream' set to lkgr or master. https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/js/article.js (right): https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:58: document.body.classList.add('no-permalink'); I would still prefer that this SCSS said: @media screen and (max-width: $break-small) { .permalink { display: none !important; } } rather than adding and removing classes in javascript. I don't think "overkill" applies here, since the media query is less code and easier to follow than the imperative javascript needed to emulate it. However, I won't insist on it. On 2014/02/05 23:58:09, ericbidelman wrote: > Yep, > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext... > is the one. I'd like to stick with this single toggle since it's already in > place. Introducing an additional media query seems overkill, especially since > this query is only evaluated once 99% of the time. > > On 2014/02/04 22:58:42, Jeffrey Yasskin wrote: > > Can you show me where? > > > https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium%25... > > returns only 4 results, all of which look like they'd be covered by this one > > media rule. > > > > On 2014/02/04 22:53:42, ericbidelman wrote: > > > It can, but .no-permalink is a generic class we're using throughout the site > > and > > > which affects other related children. > > > > > > On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > > > > Surely this can be done in the CSS, inside a media query, instead of in > > > > javascript. > > > > > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/static/js/article.js:64: // Toggle collapsible sections (mobile) On 2014/02/07 22:50:47, arthure wrote: > On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > > This is getting registered in more places than just mobile. > > > > Also, please end comments with a period so it's clear you're done with your > > thought. > > Since the "mobile" logic is based purely on width, some devices will flip back > and forth between the mobile & non-mobile views when orientation changes. > > Should we unregister and re-register the click listener in the media query > listener? My thought was that this code is running unconditionally now, so "(mobile)", in the comment, is incorrect. If you want clicks on <h2>s to toggle the 'active' class on both mobile (really "narrow") and non-mobile, this is the way to do it. If you only want that to happen on mobile, I would probably check the media query inside the event handler since clicks aren't as performance-sensitive as scrolls. https://codereview.chromium.org/138213007/diff/200001/chrome/common/extension... File chrome/common/extensions/docs/static/js/article.js (right): https://codereview.chromium.org/138213007/diff/200001/chrome/common/extension... chrome/common/extensions/docs/static/js/article.js:79: window.matchMedia('screen and (min-width: 580px)'); Looking at the uses of $break-small in the SCSS, I think this should be "min-width: 581px" to avoid applying both the mobile and desktop styles to the document at once. It's also possible that all the uses of "max-width: $break-small" in the SCSS should be "max-width: $break-small - 1" instead, since we only use "min-width: $break-small + 1" in a subset of min-width queries.
Hi Jeffery, I've fixed the problems here but I can't get the history straightened out on this CL, so I'm going to redo this on a clean branch for the sake of posterity and close this CL. Thanks, Arthur On 2014/02/07 23:37:33, Jeffrey Yasskin wrote: > Patch set 2 is a diff against patch set 1 rather than a diff against the Chrome > tree, so you won't be able to commit it. Be sure the branch you're running `git > cl upload` from has its 'upstream' set to lkgr or master. > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > File chrome/common/extensions/docs/static/js/article.js (right): > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:58: > document.body.classList.add('no-permalink'); > I would still prefer that this SCSS said: > > @media screen and (max-width: $break-small) { > .permalink { > display: none !important; > } > } > > rather than adding and removing classes in javascript. I don't think "overkill" > applies here, since the media query is less code and easier to follow than the > imperative javascript needed to emulate it. However, I won't insist on it. > > On 2014/02/05 23:58:09, ericbidelman wrote: > > Yep, > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext... > > is the one. I'd like to stick with this single toggle since it's already in > > place. Introducing an additional media query seems overkill, especially since > > this query is only evaluated once 99% of the time. > > > > On 2014/02/04 22:58:42, Jeffrey Yasskin wrote: > > > Can you show me where? > > > > > > https://code.google.com/p/chromium/codesearch/#search/&sq=package:chromium%25... > > > returns only 4 results, all of which look like they'd be covered by this one > > > media rule. > > > > > > On 2014/02/04 22:53:42, ericbidelman wrote: > > > > It can, but .no-permalink is a generic class we're using throughout the > site > > > and > > > > which affects other related children. > > > > > > > > On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > > > > > Surely this can be done in the CSS, inside a media query, instead of in > > > > > javascript. > > > > > > > > > > > https://codereview.chromium.org/138213007/diff/1/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/js/article.js:64: // Toggle collapsible > sections (mobile) > On 2014/02/07 22:50:47, arthure wrote: > > On 2014/02/04 22:22:02, Jeffrey Yasskin wrote: > > > This is getting registered in more places than just mobile. > > > > > > Also, please end comments with a period so it's clear you're done with your > > > thought. > > > > Since the "mobile" logic is based purely on width, some devices will flip back > > and forth between the mobile & non-mobile views when orientation changes. > > > > Should we unregister and re-register the click listener in the media query > > listener? > > My thought was that this code is running unconditionally now, so "(mobile)", in > the comment, is incorrect. > > If you want clicks on <h2>s to toggle the 'active' class on both mobile (really > "narrow") and non-mobile, this is the way to do it. If you only want that to > happen on mobile, I would probably check the media query inside the event > handler since clicks aren't as performance-sensitive as scrolls. > > https://codereview.chromium.org/138213007/diff/200001/chrome/common/extension... > File chrome/common/extensions/docs/static/js/article.js (right): > > https://codereview.chromium.org/138213007/diff/200001/chrome/common/extension... > chrome/common/extensions/docs/static/js/article.js:79: window.matchMedia('screen > and (min-width: 580px)'); > Looking at the uses of $break-small in the SCSS, I think this should be > "min-width: 581px" to avoid applying both the mobile and desktop styles to the > document at once. It's also possible that all the uses of "max-width: > $break-small" in the SCSS should be "max-width: $break-small - 1" instead, since > we only use "min-width: $break-small + 1" in a subset of min-width queries.
Message was sent while issue was closed.
Renato is a better person to review this than me. lgtm once he's happy. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
