Code Quality: Improved UserControls codebase #13492
Conversation
0x5bfa
changed the title
|
This is ready for review. |
| //protected override AutomationPeer OnCreateAutomationPeer() | ||
| //{ | ||
| // return new DataGridHeaderAutomationPeer(this); | ||
| //} |
There was a problem hiding this comment.
To remind devs that this control should have AutomationPeer. I will add a TODO.
| { | ||
| return FocusManager.GetFocusedElement(XamlRoot) as FrameworkElement; | ||
| } | ||
| else | ||
| { | ||
| return FocusManager.GetFocusedElement() as FrameworkElement; | ||
| } |
There was a problem hiding this comment.
Can be turned into a ternary return.
| InitializeComponent(); | ||
| PreviewPaneViewModel = Ioc.Default.GetRequiredService<PreviewPaneViewModel>(); | ||
| } | ||
| private readonly IAddItemService _addItemService = Ioc.Default.GetRequiredService<IAddItemService>(); |
There was a problem hiding this comment.
Should be named AddItemService.
There was a problem hiding this comment.
How come?
Runtime Code Style Conventions Itme 3:
We use _camelCase for internal and private fields and use readonly where possible.
Our Code Stlye Guideline Section 2, Item 4:
Use camelCase prefixed with _ for private fields
There was a problem hiding this comment.
In that case, the two services below are not named properly.
There was a problem hiding this comment.
But as my policy ones that are referenced from XAML should be named with PascalCase; Commands & ModifiableCommands because when accessing in XAML, the code will be like as follows if CommandManager is defines in code-behind.
<Button Command="Commands.OpenInNewTab" />
not
<Button Command="_commands.OpenInNewTab" /> <!-- This looks odd -->Is that not bad practice? I wanna hear from you.
There was a problem hiding this comment.
There is something off with the second format indeed, it looks really off. Yet at the same time, if we're making an exception to our coding rules, than I believe the coding rules are not good enough.
On a first read, one can be confused as to why some private fields are marked with an underscore and why others are not.
Maybe we can mark the fields used in xmal with a letter (bad idea in my opinion). Maybe we can add a rule in the coding rules about having them capitalized as a way to mark their use in XMAL (will work for a while, but give it a year or two and the whole thing gonna be a real mess).
I need to think about it a little bit to see if anything clever can be made. Maybe we can find some inputs online for good practice.
Either way, that is something minor here. We can open an issue about that to keep track of it.
Tagging @d2dyno1 since I believe he wrote the coding rules, he might have some good input about it?
There was a problem hiding this comment.
Why not just use a private property in this case?
|
I'm not so sure about renaming |
|
Which steps were used to verify this changes? |
| <ResourceDictionary Source="ms-appx:///ResourceDictionaries/App.Theme.TextBlockStyles.xaml" /> | ||
| <ResourceDictionary Source="ms-appx:///ResourceDictionaries/PathIcons.xaml" /> | ||
| <ResourceDictionary Source="ms-appx:///UserControls/SideBar/SideBarControls.xaml" /> | ||
| <ResourceDictionary Source="ms-appx:///ResourceDictionaries/TextBlockStyles.xaml" /> |
There was a problem hiding this comment.
| <ResourceDictionary Source="ms-appx:///ResourceDictionaries/TextBlockStyles.xaml" /> | |
| <ResourceDictionary Source="ms-appx:///ResourceDictionaries/App.Theme.TextBlockStyles.xaml" /> |
The . makes the code more readable.
| @@ -149,7 +150,7 @@ | |||
| </Setter> | |||
| </Style> | |||
|
|
|||
| <Style TargetType="uc:DataGridHeader"> | |||
| <Style BasedOn="{StaticResource DefaultDataGridHeaderStyle}" TargetType="uc:DataGridHeader"> | |||
There was a problem hiding this comment.
It might be better to do this in a separate PR so we can review the change on it's own.
Summary
This namespace was one of the places where there were leftovers from UWP.
Re-organized codebase in UserControls to enhance maintainability and removed some XAML files(OpacityIcon and DataGridHeader), so it must be little but the app should be faster a bit.
Task Checklist
IAddressToolBartoIAddressToolbarViewModelISearchBoxtoISearchBoxViewModelISearchBoxtoISearchBoxViewModelViews.LayoutModestoViews.ContentLayoutsViewModels.LayoutModestoViewModels.ContentLayoutsIAddressToolbarViewModelfromUserControlstoViewModels.UserControlsISearchBoxViewModelfromUserControlstoViewModels.UserControlsViewModels.PreviewstoViewModel.UserControls.PreviewsViewModels.WidgetstoViewModel.UserControls.WidgetsDataGridHeaderfromUserControltoButtonBaseKnown Issues
Following issues would be fixed in the upcoming PR.
N/A
PR Checklist
Steps To Validate Changes
There're some classes that were changed codebase largely. Please have look at:
Screenshots
N/A
Footnotes
If the request hasn't been agreed by the team, this work might be rejected. Make sure that you get approval from the team before opening any PR related the request. ↩
If you removed any en-US string resources, make sure that there are no references of those resources. When you add a new en-US string resources, do not make any changes on other languages' resources; Crowdin will do, instead. ↩