Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(md-progress-bar): Create initial ProgressBar implementation. #130

Merged
merged 1 commit into from
Apr 8, 2016
Merged

feat(md-progress-bar): Create initial ProgressBar implementation. #130

merged 1 commit into from
Apr 8, 2016

Conversation

josephperrott
Copy link
Member

For #40

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Mar 4, 2016
@josephperrott
Copy link
Member Author

I signed it!

@@ -43,3 +43,6 @@ Here's a list of gotchas to keep in mind when writing TypeScript code that will
* **Lambdas need to abide to the type required.** Meaning that if a function requires a function that takes one argument, the lambda cannot be `() => {}`. Use `_` for temporary parameters. This is notable in Promises.
* **Dynamic return values fat arrows can't return non-primitive types without casting.** For example, a fat arrow that return a promise (or Future) generates a warning if using directly, like so: `promiseA.then(() => promiseB)`. In this case simply using a block works as expected; `promiseA.then(() => { return promiseB; })`.
* **Dart does not have [generic methods](http://www.typescriptlang.org/Handbook#generics)** ([issue](https://github.com/dart-lang/sdk/issues/254)).
* **Dart does not have destructuring** but it does have a [proposal](https://github.com/dart-lang/dart_enhancement_proposals/issues/24) for it.
* **Dart does not support type aliases.** Code like `type Nop = () => void` will not compile.
* **Dart does not support [tagged template literals](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#Tagged_template_literals)**.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you need to rebase again, since these changes came from an earlier commit.

@jelbourn
Copy link
Member

jelbourn commented Mar 7, 2016

Finished first pass of comments.

}


/** Animations for indeterminate and query mode. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you point to the resource you used to get those percentage for animations? They seem like magic numbers without context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

@josephperrott josephperrott added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Mar 10, 2016

/** Animations for indeterminate and query mode. */
// Primary indicator.
@keyframes md-progress-bar-primary-indeterminate-translate {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible at all to go into more detail about what each keyframe animation is actually doing?
Perhaps @traviskaufman could add more info

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure can 😃

The indeterminate state is essentially made up of two progress bars, one primary (the one that is shown in both the determinate and indeterminate states) and one auxiliary, which essentially mirrors the primary progress bar in appearance but is only shown to assist with the indeterminate animations.

Given the mocks for the indeterminate APIs, you essentially have 4 animations being performed in tandem:

Keyframe Block Description
primary-indeterminate-translate Translation of the primary progressbar across the screen
primary-indeterminate-scale Scaling of the primary progressbar as it's being translated across the screen
auxiliary-indeterminate-translate Translation of the auxiliary progressbar across the screen
auxiliary-indeterminate-scale Scaling of the auxiliary progressbar as it's being translated across the screen

Because two different transform animations need to be applied at once, the translation is applied to the outer element and the scaling is applied to the inner element, which provides the illusion necessary to make the animation work.

Copy link
Member Author

Choose a reason for hiding this comment

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

added. I just copied the description you gave into the comment.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Mar 16, 2016
*
* Defaults to zero. Mirrored to aria-valuenow.
*/
@Input('value')
Copy link
Member

Choose a reason for hiding this comment

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

The @Input() decorator should go on the getter, otherwise angular is going to directly set the private property (and not use the setter).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I understand what you are saying here.

I dont think it makes sense to put @Input on a getter.

It currently is:

@Input('value') _value: number = 0;
@HostBinding('attr.aria-valuenow') get value_() { return this._value; }

Copy link
Member

Choose a reason for hiding this comment

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

It should be

private _value: number = 0;
...
@Input()
get value(): number {
  ...
}

set value(v: number) {
  ...
}

When the @Input is on the private member, the setter is not called when Angular updates value, which isn't what you want. Angular is smart enough to know that putting @Input on a getter or a setter means to simply treat that as the bound property.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Mar 22, 2016
@jelbourn jelbourn removed the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Mar 22, 2016
@jelbourn
Copy link
Member

Needs a fresh rebase

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Apr 8, 2016
@josephperrott
Copy link
Member Author

Rebased, PTAL

@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Apr 8, 2016
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Apr 8, 2016
@googlebot
Copy link

CLAs look good, thanks!

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Apr 8, 2016
@jelbourn
Copy link
Member

jelbourn commented Apr 8, 2016

LGTM

@jelbourn jelbourn merged commit c640f0c into angular:master Apr 8, 2016
@josephperrott josephperrott deleted the bar-only branch April 12, 2016 22:21
andrewseguin pushed a commit to andrewseguin/components that referenced this pull request Oct 15, 2018
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants