Add page indicator and progress bar polymer elements

Add page indicator and progress bar polymer elements

Add polymer elements for a page indicator and a progress bar for PDF.

BUG=303491
R=ganetsky@chromium.org

Committed: 9f76cc2

[ganetsky1, 2014-02-04 17:27:28]

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
File elements/viewer-page-indicator/viewer-page-indicator.html (right):

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:4: <div
id="text">1</div>
You can use data binding

http://www.polymer-project.org/polymer.html#data-binding-and-custom-attributes

<div id="text">{{text}}</div>

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:16:
this.style.visibility = 'visible';
Ternary operator?

this.style.visibility = (document.documentElement.clientWidth ==
window.innerWidth) ? 'hidden' : 'visible';

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:20:
document.documentElement.clientHeight);
Fix indentation

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:22: this.offsetHeight)
+ 'px';
I'd rather the entire parenthesized subexpression be on one line

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:25:
clearTimeout(this.timerId);
nitpick: you drop the if statement and just say clearTimeout(this.timerId)

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:31:
window.addEventListener('scroll', function() {
Can you use declarative event mapping?
http://www.polymer-project.org/polymer.html#declarative-event-mapping

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:32:
webkitRequestAnimationFrame(scrollCallback);
I don't know what the status of "webkit" prefixed features are, but this feels
fishy.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:38:
this.$.text.innerHTML = this.text;
Use data binding instead

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-progress...
File elements/viewer-progress-bar/viewer-progress-bar.html (right):

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-progress...
elements/viewer-progress-bar/viewer-progress-bar.html:13: <li
class="segment"></li>
I would try to avoid this repitition and just have a 'segments' constant.

Then have a segment template and stamp it out as many times as you need to.

http://www.html5rocks.com/en/tutorials/webcomponents/template/

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-progress...
elements/viewer-progress-bar/viewer-progress-bar.html:40:
this.segments[i].style.visibility = 'hidden';
Ternary operator?

this.segments[i].style.visibility = (i < numVisible) ? 'visible' : 'hidden';

[raymes, 2014-02-05 05:04:36]

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
File elements/viewer-page-indicator/viewer-page-indicator.html (right):

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:4: <div
id="text">1</div>
On 2014/02/04 17:27:29, ganetsky1 wrote:
> You can use data binding
> 
> http://www.polymer-project.org/polymer.html#data-binding-and-custom-attributes
> 
> <div id="text">{{text}}</div>

Oops. Done.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:16:
this.style.visibility = 'visible';
On 2014/02/04 17:27:29, ganetsky1 wrote:
> Ternary operator?
> 
> this.style.visibility = (document.documentElement.clientWidth ==
> window.innerWidth) ? 'hidden' : 'visible';

Done.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:20:
document.documentElement.clientHeight);
On 2014/02/04 17:27:29, ganetsky1 wrote:
> Fix indentation

Done.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:22: this.offsetHeight)
+ 'px';
On 2014/02/04 17:27:29, ganetsky1 wrote:
> I'd rather the entire parenthesized subexpression be on one line

Done.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:25:
clearTimeout(this.timerId);
On 2014/02/04 17:27:29, ganetsky1 wrote:
> nitpick: you drop the if statement and just say clearTimeout(this.timerId)

Done.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:31:
window.addEventListener('scroll', function() {
It doesn't seem so. I guess because the scroll event is never received by this
element, just the window.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:32:
webkitRequestAnimationFrame(scrollCallback);
On 2014/02/04 17:27:29, ganetsky1 wrote:
> I don't know what the status of "webkit" prefixed features are, but this feels
> fishy.

Done. - the prefix isn't needed anymore.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-page-ind...
elements/viewer-page-indicator/viewer-page-indicator.html:38:
this.$.text.innerHTML = this.text;
On 2014/02/04 17:27:29, ganetsky1 wrote:
> Use data binding instead

Done.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-progress...
File elements/viewer-progress-bar/viewer-progress-bar.html (right):

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-progress...
elements/viewer-progress-bar/viewer-progress-bar.html:13: <li
class="segment"></li>
Because it's so simple, I don't know if it's worth converting to a template. But
it seems fair to add the nodes on construction which simplifies the constructor
logic as well.

https://codereview.chromium.org/152913003/diff/70001/elements/viewer-progress...
elements/viewer-progress-bar/viewer-progress-bar.html:40:
this.segments[i].style.visibility = 'hidden';
On 2014/02/04 17:27:29, ganetsky1 wrote:
> Ternary operator?
> 
> this.segments[i].style.visibility = (i < numVisible) ? 'visible' : 'hidden';

Done.

[ganetsky1, 2014-02-05 16:55:19]

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-page-in...
File elements/viewer-page-indicator/viewer-page-indicator.html (right):

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-page-in...
elements/viewer-page-indicator/viewer-page-indicator.html:25: this.timerId =
undefined;
Just fyi, clearTimeout(undefined) or clearTimeout(timeoutId) called multiple
times on a given timeoutIt are both harmless.

Can you ever get into a situation here where you have multiple outstanding
timeouts concurrently? I think your intention is to prevent that from happening?

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
File elements/viewer-progress-bar/viewer-progress-bar.html (right):

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:16: var NUM_SEGMENTS = 8;
You could make this component more resuable and configurable by making this a
public attribute.

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:20:
this.$.segments.appendChild(segment);
It would be better if you just cloned a DOM nome instead of constructed it in
javascript. How it looks with templates:

Anywhere in the HTML:

<template id="segmentTemplate">
  <li class='segment'></li>
</template>

In the Javascript:

var template = this.$.segmentTemplate;
for (var i = 0; i < this.numSegments; ++i) {
  var segment = document.importNode(template, true);
  this.$.segments.appendChild(segment);
  this.segments.push(segment);
}

Incidentally, if you expose numSegments as an attribute... you can change this
method from ready to numSegmentsChanged... and make segments modifiable during
execution of the application! All you would have to do is clear the segments
array and this.$.segments.innerHTML = '' in this function.

That's the magic of components!

[raymes, 2014-02-07 00:33:41]

Thanks!

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-page-in...
File elements/viewer-page-indicator/viewer-page-indicator.html (right):

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-page-in...
elements/viewer-page-indicator/viewer-page-indicator.html:25: this.timerId =
undefined;
Cool, thanks!

Yep my intention is to prevent that. Because the timeout is either cleared if it
hasn't been run yet, or run, there should never be multiple timeouts
concurrently.

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
File elements/viewer-progress-bar/viewer-progress-bar.html (right):

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:16: var NUM_SEGMENTS = 8;
I had a try at doing this but the math to do the segmenting properly becomes
more complicated and there isn't really a need to expose the number of segments
at the moment. I think we could extend this in the future if need-be

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:20:
this.$.segments.appendChild(segment);
I tried to do the templating but it didn't work for some reason. The shadow dom
that got constructed with document.importNode was empty. I haven't dug into it
but I think it's just as simple in this case (more simple even?) to construct
the dom node from scratch. I did this programmatically here. I also shifted the
style here to clean up the CSS.

[raymes, 2014-02-10 01:47:44]

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
File elements/viewer-progress-bar/viewer-progress-bar.html (right):

https://codereview.chromium.org/152913003/diff/220001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:16: var NUM_SEGMENTS = 8;
I ended up changing this to be generic. Done!

[ganetsky1, 2014-02-10 20:10:03]

https://codereview.chromium.org/152913003/diff/350001/elements/viewer-progres...
File elements/viewer-progress-bar/viewer-progress-bar.html (right):

https://codereview.chromium.org/152913003/diff/350001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:35:
segmentsElement.removeChild(segmentsElement.lastChild);
You can just segmentsElement.innerHTML = ''

https://codereview.chromium.org/152913003/diff/350001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:45: -1 * (90 - angle) +
'deg)';
This seems like it would be easier to read in HTML with data binding

[raymes, 2014-02-11 02:10:16]

Thanks!

https://codereview.chromium.org/152913003/diff/350001/elements/viewer-progres...
File elements/viewer-progress-bar/viewer-progress-bar.html (right):

https://codereview.chromium.org/152913003/diff/350001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:35:
segmentsElement.removeChild(segmentsElement.lastChild);
On 2014/02/10 20:10:04, ganetsky1 wrote:
> You can just segmentsElement.innerHTML = ''

Done.

https://codereview.chromium.org/152913003/diff/350001/elements/viewer-progres...
elements/viewer-progress-bar/viewer-progress-bar.html:45: -1 * (90 - angle) +
'deg)';
I think basically to make it nicer this boils down to creating a new polymer
element for a segment. But I don't think it's worth adding that level of
abstraction for something this simple. If this element were to become more
complicated, I would agree.

[ganetsky1, 2014-02-11 23:38:16]

lgtm

[raymes, 2014-02-17 01:52:34]

Committed patchset #12 manually as r9f76cc2 (tree was closed).