-
Notifications
You must be signed in to change notification settings - Fork 266
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
feat: Allow user to control message view mode #10945
base: main
Are you sure you want to change the base?
Conversation
ae8adf6
to
97d05cd
Compare
Signed-off-by: SebastianKrupinski <[email protected]>
97d05cd
to
2c6772c
Compare
If you receive two messages that form a thread, you will still only see one entry in your INBOX with the current approach, right? |
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.
it works as described
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.
Tested and doesn't work. It's only possible to open the latest received message of a thread. I can not open earlier ones, because they are not shown in the message list. The list is still threaded.
Correct! I was not aware we stacked messages in the list, I have already made the backed and front end changes, just need to updated some backend tests, will push the update soon. |
Signed-off-by: SebastianKrupinski <[email protected]>
Signed-off-by: SebastianKrupinski <[email protected]>
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.
Looks sane
Are there any plans to skip building threads if the user doesn't use them anyway?
Signed-off-by: SebastianKrupinski <[email protected]>
@@ -124,6 +124,8 @@ public function __construct(string $appName, | |||
* @param int $cursor | |||
* @param string $filter | |||
* @param int|null $limit | |||
* @param string $sort returns messages in requested order ('newest' or 'oldest') |
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.
* @param string $sort returns messages in requested order ('newest' or 'oldest') | |
* @param IMailSearcH::sort_* $sort |
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.
We can't use IMailSearcH::sort_* here the UI sends 'newest' or 'oldest' and the value of IMailSearch::ORDER_* is 'DESC' and 'ASC'
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.
And this can't be streamlined?
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.
This was existing code for the most part, the only change to the sort was that the UI now sends the sort order vs the controller pulling the value from the database every time.
I didn't want to refactor sort as it was out of scope for this PR and would make the review more difficult.
I can address this in a follow-up if you want, or do you want it as part of this PR?
Resolves: #5913
Summary
User Setting
Administrator Settinig