-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Code Quality: Converted SidebarView to a custom control #16952
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
Conversation
|
PR looks mostly fine. A few notes:
|
|
|
Regarding 1, does lightweight styling not work for UserControls? I hope we dont aim to retemplate the entire sidebar for settings... For the second, yes some WinUI controls use PART_ (which stands for template part, i.e. parts that should be included in the control) but working on the WinUI 2 repository, I can remember only one or two controls that did that. From what I remember most controls did not use that schema, not using that prefix was way more common. |
|
Its for parts that the code relies on, and so when the control is re-templated it acts as a warning to include it in the template. It would give a warning in the designer if it was not present. |
04a076a to
8465d33
Compare
We will not do that absolutely, I'm trying to make it flexible for future use, largely taling about overring resources not the whole style, I don't think we use the same value as to the sidebar max threshold width and pane sizer position margin, etc: Plus, we're planning to publish this control to the public thru NuGet (only if everything goes well), I was wanting to appropriately apply the setting from users of that control and they may want to re-style the whole default style (aside us): <SidebarView Style="..." Background="...">
...
</SidebarView>
I see them across the WinUI 3 repo and even the newest controls (e.g. SelectorBar and TitleBar) have them. I'm convinced the reason why they add and I feel like we should use them. However, since this is a convention that is not documented in our coding guidelines, we may as well discuss whether or not later. CC @yaira2 |
|
Theres an anooying issue that persists where i didnt touch. Let me close this and re-convert again... |

Resolved / Related Issues
This is the first PR to improve the codebase of Sidebar controls in order to use this in the Settings pages and the Properties window.
The last thing we have to do for SidebarView is to remove ViewModel DP from that and create some events instead.
Steps used to test these changes
I have tested and confirmed working well.
CC: @marcelwgn