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(progressbar): specify height #1908

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

chenyuzhcy
Copy link
Contributor

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Thanks, just one minor comment about documentation.

Also this will change in 4.0.0-beta.2, as the height will be set on <div class="progress"> directly, so this will need to be updated soon.

@@ -50,12 +51,18 @@ export class NgbProgressbar {
*/
@Input() value = 0;

/**
* Height of the progress bar
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should clarify, as it will end up in the documentation:

Something like:
Height of the progress bar. Accepts any valid CSS height values, ex. '2rem'

@chenyuzhcy
Copy link
Contributor Author

what's the best way to make sure to update that when we upgrade?

@maxokorokov
Copy link
Member

I guess the easiest would be for you to realign and update current PR once #1914 will land in master. Or just test locally with beta.2

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Please align with latest Bootstrap markup:

https://getbootstrap.com/docs/4.0/components/progress/#height

@maxokorokov maxokorokov merged commit b329be9 into ng-bootstrap:master Oct 27, 2017
@maxokorokov
Copy link
Member

Thanks for contributing, @chenyuzhcy!
If you're interested, there is another progress bar feature missing (#905) that need a little design first.

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

Successfully merging this pull request may close these issues.

None yet

2 participants