Closed jmnavarro closed 10 years ago
Hi @jmnavarro
thanks a lot for your contribution.
A few comments from a (very) quick look at your pull request:
self.theme.thickness
is a bit ambiguous. I understand it refers to the label but only because I looked at the implementation. Maybe 'labelPadding
or labelOffset
is more appropriate ? I'd like to play with it first a little bit to understand how it actually works.theme
property changes in MDRadialProgressView then a notification is sent to the label to set the theme, but you are also setting the theme in the label manually in MDRadialProgressView:setTheme
. keyPath
is equal to keyThickness
and you're listening to the theme
key changes in the label but not doing anything if it actually changes ?I would like to merge this but first let me know if you want to tackle some of issues I outlined in the comments above or I'll try to find some time this weekend to make a few changes to your pull request and merge.
Marco
Hey Marco,
I fixed a typo related to your last point.
Regarding the third point (theme property in label), I removed it back. Maybe it's quite confusing having the theme property duplicated and given that we can alway get the theme from the view, it's not necessary having the theme in the label.
Regarding the rest of the issues, I'm sure you're more capable than me to find the right fix (:
Hi,
I think the best course of action is that I merge your pull request locally, modify it as I see fit and release an updated version later this week. I'll close this pull request but feel free to comment if you have any suggestions. Thanks.
:+1:
Hey!
I made few changes in order to allow changing theme attributes and make the label to apply those changes on the fly.
Let me know if you have any question!!