Skip to content

Feature: Add support for choosing the default sorting and grouping options #10596

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 36 commits into from
Dec 11, 2022
Merged

Feature: Add support for choosing the default sorting and grouping options #10596

merged 36 commits into from
Dec 11, 2022

Conversation

manfromarce
Copy link
Contributor

Resolved / Related Issues
Items resolved / related issues by this PR.

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

@QuaintMako QuaintMako added the ready for review Pull requests that are ready for review label Nov 28, 2022
@yaira2
Copy link
Member

yaira2 commented Nov 28, 2022

Locking this PR as the feature request requirements haven't been finalized. Let's continue dicussing in the issue.

@files-community files-community locked and limited conversation to collaborators Nov 28, 2022
@yaira2 yaira2 removed the ready for review Pull requests that are ready for review label Nov 28, 2022
@files-community files-community unlocked this conversation Nov 28, 2022
@yaira2
Copy link
Member

yaira2 commented Nov 28, 2022

Requirements have been sorted out

@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Nov 28, 2022
@manfromarce
Copy link
Contributor Author

Ok, if I understand correctly I need to remove the separate "override sort settings" option and use the existing Override layout option, and move the new settings to the Folders tab. Should I also move "Sort directories alongside files" which was already in Preferences?

@yaira2
Copy link
Member

yaira2 commented Nov 28, 2022

use the existing Override layout option, and move the new settings to the Folders tab.

Correct

Should I also move "Sort directories alongside files" which was already in Preferences?

Let's leave as is for now

QuaintMako
QuaintMako reviewed Nov 28, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Nov 28, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Nov 28, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Nov 28, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Nov 28, 2022
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A bunch of small propositions to make the code cleaner.

Edit: I did not see the previous returns of Yair. I'll run another review after the modifications has been done.

@manfromarce
Copy link
Contributor Author

The latest commit can be reverted if we prefer to leave the order as is.

manfromarce
manfromarce commented Nov 30, 2022
View reviewed changes
src/Files.App/ServicesImplementation/Settings/FoldersSettingsService.cs Outdated
set => Set(value);
}

public SortDirection DefaultDirectorySortDirection
Copy link
Contributor Author

@manfromarce manfromarce Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It was already used but was in LayoutSettingsService before, maybe I can just remove the parts I've added to expose the setting to the user?

QuaintMako
QuaintMako reviewed Dec 3, 2022
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

Aside of the comments still up, LGTM in general code-wise.

yaira2
yaira2 reviewed Dec 3, 2022
View reviewed changes
yaira2
yaira2 reviewed Dec 3, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Dec 11, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Dec 11, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Dec 11, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Dec 11, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Dec 11, 2022
View reviewed changes
QuaintMako
QuaintMako reviewed Dec 11, 2022
View reviewed changes
QuaintMako
QuaintMako previously approved these changes Dec 11, 2022
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

LGTM, aside of the small formatting I suggested above to remove unneeded spaces.

QuaintMako and others added 2 commits December 11, 2022 18:09
@QuaintMako
Merge branch 'main' into main
24609e2
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
1bf150f 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce manfromarce dismissed stale reviews from QuaintMako and yaira2 via 1bf150f December 11, 2022 19:41
manfromarce and others added 14 commits December 11, 2022 20:42
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
98bc28f 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
5041af4 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
f21c0da 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
2332c49 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
403b74e 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
79e9108 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
bb1b4e1 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
3ad30ef 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
13b6c4b 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
24f7bc6 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
756fd4a 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
753fad3 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@manfromarce @QuaintMako
Update src/Files.App/Strings/en-US/Resources.resw
48a2928 
Co-authored-by: Quaint Mako <110472580+QuaintMako@users.noreply.github.com>
@QuaintMako
Merge branch 'main' into main
817fa13
@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 Dec 11, 2022
@yaira2 yaira2 changed the title Feature: Add settings for default sorting and grouping options Feature: Add support for choosing the default sorting and grouping options Dec 11, 2022
@yaira2 yaira2 merged commit 5dd38f1 into files-community:main Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaira2 yaira2 yaira2 left review comments

+1 more reviewer

@QuaintMako QuaintMako QuaintMako approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Add option to set default sorting options
3 participants
@manfromarce @yaira2 @QuaintMako