Skip to content
This repository has been archived by the owner on Mar 13, 2018. It is now read-only.

Rewrite paper-progress as paper-linear-progress #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

traviskaufman
Copy link

This commit introduces a complete rewrite of paper-progress into a new
paper-linear-progress element. Features include the following:

  • New semantics regarding secondary-progress which now becomes
    buffer-value, as it's meant to be within the spec.
  • Removal of IronRangeBehavior since the linear progress element is not an
    interactive UI element, nor does it have anything to do with
    representing a range as the "range"-based input elements connote.
  • Slimmed down custom property API which exposes only the facets of the
    element meant to be modifiable in terms of the spec.
  • Implementation of indeterminate animation which adheres to spec.
  • Support for reversing the element, including all its
    transition/animations.
  • Support for a buffering state.
  • Complete and comprehensive test suite which adds full code coverage to
    the component.
  • Revamped demo page which shows all possible functionality of the
    element.

@traviskaufman
Copy link
Author

@cdata

@traviskaufman traviskaufman force-pushed the linear-progress-revamp branch 2 times, most recently from 5d76415 to 7ce9551 Compare November 30, 2015 15:24
@cdata cdata self-assigned this Nov 30, 2015
},
"devDependencies": {
"iron-component-page": "PolymerElements/iron-component-page#^1.0.0",
"test-fixture": "PolymerElements/test-fixture#^1.0.0",
"paper-button": "PolymerElements/paper-button#^1.0.0",
"web-component-tester": "*",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is orthogonal to this change, but could you please set this dependency to reflect the following configuration:
"web-component-tester": "polymer/web-component-tester#^3.4.0".

@traviskaufman
Copy link
Author

All changes made!

<template>
<style>
:host {
--height: var(--paper-linear-progress-height, 4px);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to prefix this "temp" custom property with some naming convention that makes it more specific to the paper-linear-progress use case. The likelihood that the current name will actually cause a problem is very small, but from a conventions perspective it would be better to set a good example.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about --element-height? is there an example of prefixing custom properties in the way you specified?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really - you are actually the first person to create temp custom properties this way that I can think of. The concern is that we don't want to accidentally assign a value to a property that will cascade into a child element and be applied unintentionally. --element-height is probably fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--element-height it is :)

@notwaldorf
Copy link

I'm just going to jump in here and plug iron-demo-helpers, which is a neat little element we're using to clean up demos. I think it would make the demo a little nicer, because you can show both the code, and the demo at the same time. Here's for example, how it cleaned up the checkbox demo:
screen shot 2015-12-01 at 11 34 40 am

Let me know if you think that can work for you!

});
});

function flushPromise() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful for these helpers to appear towards the top of the suite, or else in a separate helpers.js.

@cdata
Copy link

cdata commented Dec 4, 2015

Web Component Tester includes a feature that allows for easy a11y auditing via our test suites. It would be nice to see this used in this element's test suite. Adding it is super easy. Here is an example from paper-checkbox: https://github.com/PolymerElements/paper-checkbox/blob/master/test/basic.html#L121-L123

Basically, you pick one or more appropriate <test-fixture> elements (or make new ones as needed), and then call a11ySuite('IdOfTheTestFixture'); and WCT will run the a11y audits from Accessibility Developer Tools against those fixtures and fail the suite if there are any critical failed audit tests.

@cdata
Copy link

cdata commented Dec 5, 2015

FYI I have begun Polymer/polymer#3163 in order to potentially address the @keyframes custom properties problem described in Polymer/polymer#2399.

If this lands in short order, it would be nice if we could remove the dependency on neon-animations.

@traviskaufman
Copy link
Author

@notwaldorf added iron-demo-helpers to demo. @cdata added a11ySuite to tests. I agree that getting rid of neon-animations would be great, but I'd prefer not to wait too long for it to get merged, given that this PR has already been open for ~2weeks. :) Also, we could always address removing neon-animation in another PR! Changing from neon to CSS anims seems like a it would be patch version bump only.

@cdata
Copy link

cdata commented Dec 8, 2015

Changing from neon to CSS anims seems like a it would be patch version bump only.

@traviskaufman Unfortunately, this is not true. Removing Polymer.NeonAnimationRunnerBehavior cuts out a non-trivial amount of public API and configuration, so it would definitely be a major version bump (and in our case, yet-another-fork) if we wanted to remove it.

Thanks for your patience while I work to unblock this.

@traviskaufman
Copy link
Author

Ah right. Okay SG.

@traviskaufman
Copy link
Author

@cdata updated with indeterminate animation that works across all browsers.

This commit introduces a complete rewrite of `paper-progress` into a new
`paper-linear-progress` element. Features include the following:

- New semantics regarding `secondary-progress` which now becomes
  `buffer-value`, as it's meant to be within the spec.
- Removal of `IronRangeBehavior`, since the linear progress element is not an
  interactive UI element, nor does it have anything to do with
  representing a range as the "range"-based input elements connote.
- Slimmed down custom property API which exposes only the facets of the
  element meant to be modifiable in terms of the spec.
- Implementation of indeterminate animation which adheres to spec.
- Support for reversing the element, including all its
  transition/animations.
- Support for a buffering state.
- Complete and comprehensive test suite which adds full code coverage to
  the component.
- Revamped demo page which shows all possible functionality of the
  element.
@traviskaufman
Copy link
Author

Any updates on this lately? Given that this PR has been out for almost two months I'd love to be able to merge it in! 😄

@cdata
Copy link

cdata commented Feb 9, 2016

Polymer/polymer#3163 finally merged. The release is still pending. In the mean time, it looks like this branch needs to be rebased (most likely due to script-generated artifacts that get automatically merged into master branch).

@traviskaufman
Copy link
Author

@cdata I pulled v1.3.0 and replaced all of the NeonAnimationBehavior code with the correct CSS rules which are now supported (woo!). However, I think Polymer may be rewriting some style properties the wrong way?

In my source code I have

:host(.buffering.reversed) #bufferingDots {
  -webkit-animation: buffering-reverse 250ms infinite linear;
  animation: buffering-reverse 250ms infinite 
}

/* ... */

@keyframes buffering-reverse {
  to {
    -webkit-transform: translateX(calc(2.5 * var(--element-height)));
    transform: translateX(calc(2.5 * var(--element-height)));
  }
}

and here's a screenshot of the output in DevTools

screen shot 2016-02-25 at 11 02 07

Notice how the animation-name is buffering-paper-linear-progress-0-reverse but the keyframe is named buffering-reverse-paper-linear-progress-0. This is also happening with another keyframe rule with the suffix -reverse. EDIT: This seems to be happening with any keyframe rule that has the same prefix as a different keyframe, e.g. translate vs. translate-reverse.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants