-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Code Quality: Refactored the code for Quick Access #16485
base: main
Are you sure you want to change the base?
Conversation
0fabe5a
to
2dbf6f4
Compare
0c973ed
to
2ebb917
Compare
@@ -96,7 +96,8 @@ protected override void OnNavigatedTo(NavigationEventArgs eventArgs) | |||
|
|||
if (!string.IsNullOrEmpty(pathRoot)) | |||
{ | |||
var rootPathList = App.QuickAccessManager.Model.PinnedFolders.Select(NormalizePath) | |||
var service = Ioc.Default.GetRequiredService<IQuickAccessService>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the service be defined at the top of the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments
Task SaveAsync(string[] items); | ||
IReadOnlyList<INavigationControlItem> Folders { get; } | ||
|
||
event EventHandler<NotifyCollectionChangedEventArgs>? PinnedFoldersChanged; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this event limited to pinned folders, or does it also notify when recent folders are added to the list?
The most recent test I ran on this branch had an issue where pinned folders weren't displaying in the sidebar. |
This isn't really ready for review yet. I need to make the update pinned files async to return the pinned files and create a file notifier for the pinned files folder to use it in sidebar and the widget. |
Resolved / Related Issues
Steps used to test these changes