-
Notifications
You must be signed in to change notification settings - Fork 286
fix: sorting for older grouping #11691
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
Conversation
Signed-off-by: greta <gretadoci@gmail.com>
Signed-off-by: greta <gretadoci@gmail.com>
const groupOrder = sortOrder === 'newest' | ||
? ['lastHour', 'today', 'yesterday', 'lastWeek', 'lastMonth', 'older'] | ||
: ['older', 'lastMonth', 'lastWeek', 'yesterday', 'today', 'lastHour'] |
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.
for sorting by oldest it will not show the year of the messages, will it? it will start with Older.
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.
yes, i think older is good enough to fix the issue reported, we dont have to change it, unless someone has a strong opinion. In the ticket itself I proposed year, because i thought thats what we already have.
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.
also, changing the whole logic of showing years, then months, then week/recent, is more of a feature request, and here im just fixing a bug :)
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.
IMO it's very strange to start with Older, because it's missing a context of what it's compared to. If anything, it should start with Old. But that's weird.
What is shown in Outlook and others?
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.
Outlook has it with Years, then Months(Jan..Feb..), than last week, and today. When we worked on the initial feature, Older was proposed for messages older than last month.
I can change the scope of the PR, because yes, its odd to start with Older, but that will need a bit of more work than just fixing the bug reported.
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.
Sounds like a plan
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.
The feature was always about covering all sorting options. See #11057.
Signed-off-by: greta <gretadoci@gmail.com>
Started from scratch here: #11708 |
fixes #11567