Skip to content

Conversation

@0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Mar 26, 2025

Resolved / Related Issues

  • Decoupled some style oerrides to an indivisual Resource Dict
  • Removed an unnecessary Grid xaml tree
  • Removed unnecessary DI'd instance pass to NavToolbar from MainPage

image

Steps used to test these changes

  1. Just build, I did not change any bit of logic in Files.

@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Mar 26, 2025
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Mar 26, 2025
@yaira2 yaira2 requested a review from Copilot March 27, 2025 01:24
@yaira2 yaira2 merged commit 72c3faf into files-community:main Mar 27, 2025
6 checks passed
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the code quality of NavigationToolbar.xaml by decoupling style overrides into a dedicated resource dictionary, removing unnecessary UI elements, and eliminating redundant DI of the StatusCenterViewModel.

  • Changed public properties to private readonly fields to improve encapsulation and code clarity.
  • Refactored element type checking using pattern matching for better readability.
  • Removed removal of redundant DI instance in MainPage to simplify dependency usage.

Reviewed Changes

Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.

File Description
src/Files.App/UserControls/NavigationToolbar.xaml.cs Updated DI field declarations to be private readonly and improved type checking logic.
src/Files.App/Views/MainPage.xaml.cs Removed the redundant DI instance of StatusCenterViewModel to streamline the MainPage dependencies.
Files not reviewed (3)
  • src/Files.App/Styles/StatusCenterInfoBadgeStyles.xaml: Language not supported
  • src/Files.App/UserControls/NavigationToolbar.xaml: Language not supported
  • src/Files.App/Views/MainPage.xaml: Language not supported
Comments suppressed due to low confidence (3)

src/Files.App/UserControls/NavigationToolbar.xaml.cs:23

  • [nitpick] Consider renaming the field 'MainPageViewModel' to follow private field naming conventions (e.g., _mainPageViewModel) to clearly differentiate it from properties.
private readonly MainPageViewModel MainPageViewModel = Ioc.Default.GetRequiredService<MainPageViewModel>();

src/Files.App/UserControls/NavigationToolbar.xaml.cs:24

  • [nitpick] Consider renaming the field 'Commands' to follow naming guidelines for private fields (e.g., _commands) to enhance readability and maintain consistency.
private readonly ICommandManager Commands = Ioc.Default.GetRequiredService<ICommandManager>();

src/Files.App/UserControls/NavigationToolbar.xaml.cs:25

  • [nitpick] Consider renaming the field 'OngoingTasksViewModel' to follow naming conventions for private fields (e.g., _ongoingTasksViewModel) to prevent potential confusion with public properties.
private readonly StatusCenterViewModel OngoingTasksViewModel = Ioc.Default.GetRequiredService<StatusCenterViewModel>();

@0x5bfa 0x5bfa deleted the 5bfa/CQ-NavToolbarXamlRefactoring branch March 27, 2025 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants