-
Notifications
You must be signed in to change notification settings - Fork 13.4k
feat(progress): adds progress bar #12010
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(progress): adds progress bar #12010
Conversation
This looks visually great, both LTR and RTL. I wonder though, is progress bar the same on all platforms? (ios/md/wp) if not, there should be |
// -------------------------------------------------- | ||
|
||
/// @prop - The color of the element that is being animated | ||
$progress-color:rgb(16,108,200) !default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something lint won't catch, is variables alignment. Please keep all variable values aligned at the same column
background-color: $progress-color; | ||
transition: width $progress-values-transition cubic-bezier(0.4,0.0,0.2,1); | ||
} | ||
.bufferCircles { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if linting will catch this, but it should be buffer-circles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @AmitMY here, please use kebab-case for CSS classes.
width: 100%; | ||
|
||
display: none; | ||
mask: url("data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIj8+Cjxzdmcgd2lkdGg9IjEyIiBoZWlnaHQ9IjQiIHZpZXdQb3J0PSIwIDAgMTIgNCIgdmVyc2lvbj0iMS4xIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciPgogIDxlbGxpcHNlIGN4PSIyIiBjeT0iMiIgcng9IjIiIHJ5PSIyIj4KICAgIDxhbmltYXRlIGF0dHJpYnV0ZU5hbWU9ImN4IiBmcm9tPSIyIiB0bz0iLTEwIiBkdXI9IjAuNnMiIHJlcGVhdENvdW50PSJpbmRlZmluaXRlIiAvPgogIDwvZWxsaXBzZT4KICA8ZWxsaXBzZSBjeD0iMTQiIGN5PSIyIiByeD0iMiIgcnk9IjIiIGNsYXNzPSJsb2FkZXIiPgogICAgPGFuaW1hdGUgYXR0cmlidXRlTmFtZT0iY3giIGZyb209IjE0IiB0bz0iMiIgZHVyPSIwLjZzIiByZXBlYXRDb3VudD0iaW5kZWZpbml0ZSIgLz4KICA8L2VsbGlwc2U+Cjwvc3ZnPgo="); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can use svg-background-image
instead of mask
, please do.
If not, can you do:
`mask: url("data:image/svg+xml;charset=utf-8,#{url-encode($progress-mask-svg)}")``
Where $progress-mask-svg
is your svg, or a scalable version of it like:
<svg xmlns='http://www.w3.org/2000/svg' viewBox="0 0 12 4">
<ellipse cx="2" cy="2" rx="2" ry="2">
<animate attributeName="cx" from="2" to="-10" dur="0.6s" repeatCount="indefinite" />
</ellipse>
<ellipse cx="14" cy="2" rx="2" ry="2" class="loader">
<animate attributeName="cx" from="14" to="2" dur="0.6s" repeatCount="indefinite" />
</ellipse>
</svg>
src/components/progress/progress.ts
Outdated
|
||
@Component({ | ||
selector: 'ion-progress', | ||
templateUrl : './progress.html', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead, use template
and put your template inline, like the other components
src/components/progress/progress.ts
Outdated
constructor( ) { } | ||
|
||
/** | ||
* @input {'determinate' | 'indeterminate' | 'buffer' | 'query'} The current mode of the progress indicator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add (above the class): export type progressIndicator = 'determinate' | 'indeterminate' | 'buffer' | 'query'
and use it instead of explicit strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AmitMY I believe query
is both determinate and indeterminate. It's been awhile since I looked at these but if I recall correctly it displays as indeterminate
during the call for data and then determinate
once it gets the data and is still loading. See here:
I like using the name query
, it's short and goes with the MD guidelines. Maybe we just need to document what each indicator does really well.
src/components/progress/progress.ts
Outdated
* @input {'determinate' | 'indeterminate' | 'buffer' | 'query'} The current mode of the progress indicator | ||
*/ | ||
@Input() | ||
@HostBinding('class') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of using HostBinding
, usually they use host
on Component
, see range
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the angular docs its best to use Hostbinding
in contrast with host. But if you would like me to use host
instead, I have no problem in doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host
is used across the framework. Perhaps the team can change it in v4, but I think it is best to keep everything to the same standard. @brandyscarney what is your take on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah can we use host
please just to keep it consistent? Also with v4 we will most likely end up using host
anyways so it will be easier to port the component. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! No problems, consistency is definitely important.
RTL support is not implemented fully correctly, but that is because I introduced something new to flipping components here - #12004. When that is merged, I'll give you tips on how to improve on what you already did. (it works, but we have a mixin for that) |
I have two tests file locally that just need to be committed, whats the test command for running e2e tests and unit tests though? And I have a strange feeling you are referring to the |
@realappie To run unit tests run About RTL, yeah, you did correctly in general, I just meant it can be refined. I will give more feedback after linting fixes are done. |
0% { | ||
width: 0%; | ||
|
||
left: 0%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of left
, use:
@include multi-dir() {
// scss-lint:disable PropertySpelling
left: 0%;
}
same for all keyframes.
*It is needed for if any class wants to override it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright I will incorporate that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks my animation :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, makes sense, can you just add that comment then? Otherwise it won't lint.
@realappie this looks awesome! and the feedback from @AmitMY is absolutely correct. One thing we need to consider before merging this, is the design of the progress bar for iOS and WP, maybe @bensperry can help here. Also, I think |
Take the simple fab component as an example: Also, notice we avoid using https://github.com/ionic-team/ionic/blob/master/src/components/fab/fab.scss#L22 |
@@ -0,0 +1,4 @@ | |||
<div class="progress" [style.width.%]="value"></div> | |||
<div class="animtable"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I made the same typos multiple times ops, its supposed to be animatable
rather than animtable
This looks great! Great work. 😄 I responded to a few comments with some feedback. @AmitMY has left some good feedback, as well. @manucorporat I spoke to Ben about this awhile ago when I first started looking into it, and I was instructed to get Material Design looking right and match that for the other modes. We can always tweak styling of other modes later. |
Thanks for your valuable feedback everyone, I will be improving the the component. @brandyscarney How do I run e2e tests? Not the ones in the browser but the ones in the |
@realappie The only way we use those files is to include the tests in our snapshot command (I wrote a blog on this here). I usually run the following commands:
Although you need the snapshot key and a retina screen in order to run it accurately: https://github.com/ionic-team/ionic/blob/master/scripts/README.md#running-snapshot Are you in our worldwide slack group? https://ionicworldwide.herokuapp.com/ |
Little update on the progress bar, I have been very busy with other things the past two weeks and will have it finished this week :) |
@brandyscarney I want to write some tests in karma, how do I run those tests only for the progress component? |
$progress-color: rgb(16,108,200) !default; | ||
|
||
/// @prop - The background color of the progress bar | ||
$progress-buffer-color: rgb(170,209,249) !default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please align the value and the value above to the same column as the value below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values are aligned? Or do you want me to align the word default
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, you're right. It seemed on the same line because of vscode's color preview 😂. It renders this block on the left of each valid CSS color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha Microsoft strikes again :P
// -------------------------------------------------- | ||
|
||
/// @prop - The color of the element that is being animated | ||
$progress-color: rgb(16, 108, 2001) !default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a 1
accidentally added at the end here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update this and then after some testing we could get it merged? Is it ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently material only, and there is no separation of .scss
and .md.scss
, which I at least feel is needed for it not to be changed after being merged (all the classes names)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, yes you are right. It would probably be good to at least make the progress.mode.scss
files for determining the color of the progress bar per mode, and then using our color functions for the primary and buffer color.
In order for this to work though we need to change some things in the Progress class so that the color classes will be added. This causes other issues because of the progress
class in the template. I'm thinking we should rename the template classes to be more specific (also makes it less likely to have conflicts with user's classes).
I created a commit for these changes here: cfb3972
Naming is up for discussion if any of them don't make sense, but it reduces nesting as well in the Sass files. I'm sure some improvements can be made there, let me know your thoughts.
@realappie Are you on Slack? If you send me or comment your email I could invite you to ours for discussion.
Hey guys, how come this is taking so long when the PR is already created? Can we get remaining issues fixed soon? I am waiting on this feature. I really appreciate the work and hope to see this merged soon! 💯 |
I'm also really interested to see the progress bars being finally added to ionic. As it seems, everything is done and ready to be merged? |
Just wanted to update everyone on why some PR's haven't been merged. We've been hard at work on the mono-refactor branch which is eventually going to turn into our v4 release. There are a lot of changes coming that will have major performance improvements and fix some longstanding issues. We've touched on this in the "What's next" section of our blog post here. With v4 comes some big changes internally, so every change done to master can't be simply pulled to the branch, it has to be manually ported. Unfortunately, this has caused us to slow down on merging some changes to master for now. I do want to get this PR merged though before the v4 release. I added some comments above with some feedback and a commit of some partial changes that should be implemented to get it working with colors & modes. |
@brandyscarney Good working on cfb3972. I have actually been on slack for a while now under |
@brandyscarney Are you already migrating all the ionic 3.x components to stencil branch internally? Or should I pick that up. |
@realappie See https://github.com/ionic-team/ionic/tree/mono-refactor |
@AmitMY Is that a yes or no? :P And do you have any slack channel for contributors? |
Yep, they are already migrating the ionic components to stencil in that branch. |
Sorry, yes @realappie we have started porting components. I sent you a message on the worldwide slack. If you could send me your email there or post it here (I can delete the comment afterwards) I can add you to our company slack for discussion. Thanks! 😄 |
Perhaps this could be added for 3.9.0? |
I moved it into |
@kensodemann I will see if I can re-write it :) Should I try and get it into 3.9 or just rewrite it in the stencil branch? Whats the best thing to do here. |
@realappie Nothing official, but the way I see it, new stuff were not merged into master for many moons now, and only the |
@realappie - I changed the milestone to 4.x My suggestion is that you see if you can target v4 and the |
@kensodemann I wouldn't mind another 3.x release, I think building for 4.x would be the best thing to do at the moment. I will see if I can make some time for it this weekend! |
We updated the |
Short description of what this resolves:
This resolves #6669
Changes proposed in this pull request:
This PR has some SCSS linting errors, I hope @brandyscarney can help out here :)
Ionic Version: 2.x