GradientSlider component creation#769
Conversation
|
This is so cool! 🦙❤️ Thanks for thinking of this and putting this together, excited to go peek at how you've put it together in a minute. Did a quick check on the CI, seems like some of the namespaces didn't get globalized properly for WinUI 2: |
michael-hawker
left a comment
There was a problem hiding this comment.
Some initial comments, this is an amazing start! Really interesting use of the Canvas. I'll have to pull it down and play with it a bit more directly.
I'm also wondering since I added Thumbs to the ResizeAdorner, and you have them here if there's any commonalities and if we just need a general Thumb base class like I did for the Sizers (which could honestly maybe re-use that as well, I mean I basically took that code and made it two-dimensional for the Adorner thumb).
| <controls:GradientSlider.GradientStops> | ||
| <GradientStopCollection> | ||
| <GradientStop Offset="0" Color="#FF0000" /> | ||
| <GradientStop Offset="0.3" Color="#00FF00" /> | ||
| <GradientStop Offset="0.66" Color="#0000FF" /> | ||
| <GradientStop Offset="1" Color="#FF0000" /> | ||
| </GradientStopCollection> | ||
| </controls:GradientSlider.GradientStops> |
There was a problem hiding this comment.
What happens if the developer doesn't specify any stops at the start? Could be good to have another example where it's bound to a collection (to show binding) but that collection is also empty to start?
There was a problem hiding this comment.
Seconded, documenting (and defining if needed) the expected behavior for empty/default properties is a trivial known/expected scenario for any component MVP.
components/GradientSlider/src/CommunityToolkit.WinUI.Controls.GradientSlider.csproj
Outdated
Show resolved
Hide resolved
| if (slider._isDragging) | ||
| return; |
There was a problem hiding this comment.
So if a stop is somehow programmatically added while the slide is in use, the slider will never get updated and be out-of-sync? We probably need to call RefreshThumbs or some other sync method when the drag is completed, eh?
There was a problem hiding this comment.
Seconded. This is another known/expected trivial MVP feature, the various properties on this control should support live updates.
We'll likely need to do a sweep to make sure property change updates are wired everywhere.
There was a problem hiding this comment.
It's actually worse than that.
There is no event or callback on the GradientStopCollection to detect a stop being added/removed. The actual limitation here is that if the GradientStops property values changes (as in the reference to the GradientStopsCollection object, while the user is dragging then it will become completely out of sync until the next RefreshThumbs() event.
| <FlyoutBase.AttachedFlyout> | ||
| <Flyout Placement="Bottom"> | ||
| <muxc:ColorPicker x:Name="PART_ColorPicker" /> | ||
| </Flyout> | ||
| </FlyoutBase.AttachedFlyout> |
There was a problem hiding this comment.
I'm always leery of adding intertwined children/parents, but I think in this case, it'd be nice if there was a single attachedflyout was part of the parent control and the child invoked that atop itself? Obviously, requires a bit more coordination, but...
Then I think it gets easier to expose the properties of the ColorPicker itself in one place on the GradientSlider. i.e. if they want to show a specific color palette or other things instead, they just have to restyle the parent control which is much simpler than the thumb.
Should we also use the WCT ColorPicker by default instead? Or should we expose a ColorPicker property on the slider so the developer themselves provide the picker they want to use configured exactly how they want (we can have a style value set with a default basic one in case they don't provide one)?
There was a problem hiding this comment.
Just to make sure I understand what you're saying, we shouldn't have to retemplate the entire control to swap out the color picker or add any custom content around it? That instead, we could pass it in via a DP deriving ColorPicker, or if we wanted to allow custom content, a Control instance or DataTemplate?
There was a problem hiding this comment.
@Arlodotexe Yes, but as a result the GradientSlider would own the ColorPicker. This has a cost of higher control coupling, but it is more layout optimized and makes this control more easily extensible (even though its components become less extensible).
There was a problem hiding this comment.
I would think, double-tapping on the slider should be the mechanism to add a new stop, eh? @niels9001 any suggestions on input here?
I'm trying to think of how you'd remove a stop, would it be dragging it further off some amount the end of the slider?
The only other scenario I'm thinking of here that'd be more complex to add is if you wanted a specific input box for the offset vs. drag input. (It could be nice in either case as you're dragging to see the offset value as a float [0-1] and/or percentage maybe?) Maybe it's not something we provide by default out-of-the-box, but something we should have a sample on how to extend the control to enable. i.e. could the flyout be customized to contain more than just a colorpicker, would we have to search the flyout for the colorpicker instance then?
There was a problem hiding this comment.
I'm a fan of the hover-to-add interaction method, though it doesn't cover removal. We can defer addressing both of these until later, but let's track it in a new ticket after closing this PR.
There was a problem hiding this comment.
Currently you can right-click to remove a stop. Not the most intuitive solution, but just making clear that there is a way to remove stops right now.
Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
| /// An example sample page of a custom control inheriting from Panel. | ||
| /// </summary> | ||
|
|
||
| [ToolkitSample(id: nameof(GradientSliderSample), "Custom control", description: $"A sample for showing how to create and use a {nameof(GradientSlider)} custom control.")] |
There was a problem hiding this comment.
Default text (name/description) should be replaced so it's displayed properly in the sample app.
There was a problem hiding this comment.
Reviewed and approved, there are improvements to make but nothing we can't defer. Let's take the lingering review comments and turn them into tracking issues, once we've resolved the review threads by deferring them to issues, we can merge the code and close the PR.
Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>
|
I've taken care of the simpler problems (default strings and metadata) |
|
@Arlodotexe seems like WIP is stuck again... 🤔 I confirmed our installation is this repo: https://github.com/wip/action - they have a status here which shows it's been up: https://stats.uptimerobot.com/Dq46zf6PY - maybe we should file an issue anyway? I'm not sure how we get more details about what's holding it up though (it's a simple action and it hasn't changed). |

Introduces the first draft of the
GradientSlidercomponent.Resolves: #768
GradientSlider.mp4