Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683123002:
MD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row. (Closed)
|
Created:
3 years, 10 months ago by fukino Modified:
3 years, 10 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row.
The size of other users is accessed via this.$.otherUsersSize.
This CL uses hidden attribute instead of dom-if to ensure the registration.
BUG=690337
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2683123002
Cr-Commit-Position: refs/heads/master@{#449494}
Committed: https://chromium.googlesource.com/chromium/src/+/cab47b8c00b8263555fae1c718dd9046541902f7
Patch Set 1 #Patch Set 2 : Use dom-if to avoid unnecessary load. #Patch Set 3 : Remove unncecessary change. #
Total comments: 3
Messages
Total messages: 20 (7 generated)
Description was changed from ========== MD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row. BUG= ========== to ========== MD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row. The size of other users is accessed via this.$.otherUsersSize. This CL uses hidden attribute instead of dom-if to ensure the registration. BUG=690337 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
fukino@chromium.org changed reviewers: + stevenjb@chromium.org
Hi Steven, could you take a look? Thanks!
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
just change this.$.otherUsersSize to this.$$('#otherUsersSize')
On 2017/02/09 16:17:21, Dan Beam wrote:
> just change this.$.otherUsersSize to this.$$('#otherUsersSize')
Is it a preferred way?
I use hidden attribute and this.$.foo in every row, so I prefer
this.$.otherUserSIze for consistency.
But, if this.$$ is recommended, I'll update how to access the elements in every
row.
On 2017/02/09 18:13:38, fukino wrote:
> On 2017/02/09 16:17:21, Dan Beam wrote:
> > just change this.$.otherUsersSize to this.$$('#otherUsersSize')
>
> Is it a preferred way?
> I use hidden attribute and this.$.foo in every row, so I prefer
> this.$.otherUserSIze for consistency.
> But, if this.$$ is recommended, I'll update how to access the elements in
every
> row.
dom-if has the advantage that the html does not get stamped until/unless the
condition is true, which helps with load times.
We use this.$$() pretty frequently in the code to address the issue that you ran
into, and I think that even though !isGuest is the much more common condition,
it is still better practice to use dom-if here (and will slightly reduce load
times for guest users).
On 2017/02/09 18:13:38, fukino wrote:
> On 2017/02/09 16:17:21, Dan Beam wrote:
> > just change this.$.otherUsersSize to this.$$('#otherUsersSize')
>
> Is it a preferred way?
> I use hidden attribute and this.$.foo in every row, so I prefer
> this.$.otherUserSIze for consistency.
> But, if this.$$ is recommended, I'll update how to access the elements in
every
> row.
the local ID object (this.$) only includes things around when stamped (as
stevenjb mentioned).
it's not a "conventional" difference, it's a functional difference (as in: your
code doesn't work with dom-if + this.$.localId).
if we're not gonna show this value in guest mode, can we also not set up
listeners and stuff?
Thank you Steven, Dan for the explanation. I replaced all "hidden$=" with dom-if to avoid the unnecessary load, and stop listening size of other users in guest mode. For androidSize and driveSize, I keep the listeners since they can be enabled after the element's ready() Could you take another look?
Thanks for adding the extra templating, lgtm
https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/storage.html (right): https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/storage.html:131: <div class="message-area"> nit: These divs can probably be combined? https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/storage.html:145: <div id="criticallyLowMessage" class="message-area"> nit: These divs too?
https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/storage.html (right): https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/storage.html:131: <div class="message-area"> On 2017/02/09 20:21:00, stevenjb wrote: > nit: These divs can probably be combined? I think the dom tree for these two warnings can be unified, but can I do the refactoring in another CL?
On 2017/02/09 20:42:57, fukino wrote: > https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/device_page/storage.html (right): > > https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/device_page/storage.html:131: <div > class="message-area"> > On 2017/02/09 20:21:00, stevenjb wrote: > > nit: These divs can probably be combined? > > I think the dom tree for these two warnings can be unified, but can I do the > refactoring in another CL? Yep, that's thy they are nits :)
On 2017/02/09 23:35:19, stevenjb wrote: > On 2017/02/09 20:42:57, fukino wrote: > > > https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/settings/device_page/storage.html (right): > > > > > https://codereview.chromium.org/2683123002/diff/40001/chrome/browser/resource... > > chrome/browser/resources/settings/device_page/storage.html:131: <div > > class="message-area"> > > On 2017/02/09 20:21:00, stevenjb wrote: > > > nit: These divs can probably be combined? > > > > I think the dom tree for these two warnings can be unified, but can I do the > > refactoring in another CL? > > Yep, that's thy they are nits :) Thanks!
The CQ bit was checked by fukino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486683424707260,
"parent_rev": "cd03f64dcc04f08e8ed35b058fe92bba1bd9abb1", "commit_rev":
"cab47b8c00b8263555fae1c718dd9046541902f7"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row. The size of other users is accessed via this.$.otherUsersSize. This CL uses hidden attribute instead of dom-if to ensure the registration. BUG=690337 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Storage: Use hidden attribute instead of dom-if binding to hide a row. The size of other users is accessed via this.$.otherUsersSize. This CL uses hidden attribute instead of dom-if to ensure the registration. BUG=690337 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2683123002 Cr-Commit-Position: refs/heads/master@{#449494} Committed: https://chromium.googlesource.com/chromium/src/+/cab47b8c00b8263555fae1c718dd... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/cab47b8c00b8263555fae1c718dd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
