-
-
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
Feature: Added support for grouping items by month #12288
Conversation
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.
In terms of codebase. Otherwise LGTM!
src/Files.App/ServicesImplementation/DateTimeFormatter/AbstractDateTimeFormatter.cs
Outdated
Show resolved
Hide resolved
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.
Last thing.
Can you not collapse by the reviewee-self other than 'Suggested changes'? Reviewers cannot see what you say or how you respond. |
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.
LGTM
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.
Otherwise LGTM
Would it be better to have "Group by month" be its own option instead of making it a sub option of "Group by date"? |
I think so. There are several "Group by date" options, so it would be cleaner to add one sub option than to add monthly groupings to all of them. |
That's true, but it's still confusing to have the sub options the way they are. While I dislike nesting multiple sub menus, I think it's the best option here. |
True, you don't see which one is selected without opening the menu but at least the options are grouped together. It's not apparent that the options go together if they aren't next to each other. |
I changed "Group by year" and "Group by month" to sub options of "Group by date". |
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.
LGTM
Also fixed a minor bug where checkmarks for sort direction were not shown in the flyout.
Resolved / Related Issues
Closes Feature: Add support to group by month #12244
Validation
How did you test these changes?
Screenshots