Add windows progress bar #37
Conversation
example/windows/ProgressViewExample/ProgressViewExample.vcxproj
Outdated
Show resolved
Hide resolved
|
General note, before you merge this, since you have pushed many commits, do a squash merge in order to reduce clutter in git log. |
🎉
| namespace winrt::progress_view::implementation { | ||
|
|
||
| ProgressViewView::ProgressViewView(winrt::IReactContext const& reactContext) : m_reactContext(reactContext) { | ||
| this->Minimum(0); |
There was a problem hiding this comment.
are these hardcoded values what the module specifies as default?
There was a problem hiding this comment.
The module specifies the progress value to be between 0 and 1. By default the min/max of XAML progress bar is 0/100 but I left it in for readability. Would it be better to take it out?
There was a problem hiding this comment.
Not sure about this one. If these are the default XAML values, you don't need to set them explicitly yourself.
If you want to keep the XAML defaults, you can a comment saying what they are. Still, it's probably better to put it as a default prop value on the JS side. You should also consider, if the default should be the same as for other platforms (as defined by this module) or whether we'd like the XAML one to override it. It's q question you could ask the maintainers when they review this PR.
Perhaps @asklar could help, as well.
| } | ||
| else if (propertyName == "progressViewStyle") { | ||
| if (propertyValue == "bar") { | ||
| this->Background(SolidColorBrush(ColorHelper::FromArgb(000, 000, 000, 000))); |
There was a problem hiding this comment.
Can there be other color values when using this prop?
There was a problem hiding this comment.
There wasn't much info on this property (The description being "The progress bar style.") but by the picture I assumed it was making the background transparent. There could be other color values but the property 'trackTintColor' handles that.
There was a problem hiding this comment.
Yeah, I get that. The maintainers might be able to clarify that, too.
|
@Naturalclar this PR has been reviewed by my coworkers! Let me know if I need to change anything to the PR :) |
There was a problem hiding this comment.
@TatianaKapos Thank you so much! I'll merge it and release a new version soon.
|
Released 1.1.0 with this change 🎉 |
Summary
Added windows support to progress view
Added property
isIndeterminatespecifically for windowsCloses microsoft/react-native-windows#4389
Compatibility
Checklist
README.mdCHANGELOG.mdexample/App.js)