-
-
Notifications
You must be signed in to change notification settings - Fork 286
feat: change names of groups from Priority Field and change the sort of order groups same as task sorting order #1966
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
Changes from 15 commits
d7fc0dc
1acce88
1e7e92c
edca28f
3f1cb45
5e827e4
e9355f2
d0a078f
090c195
f61c819
1d29d1d
bf597ea
fefd7b2
3180630
11c12be
b7f9f20
67a5baf
65d132c
8c2adc3
75e6cf7
a5c389c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,7 +64,7 @@ export abstract class MultiTextField extends TextField { | |
* This overloads {@link Field.createGrouper} to put a plural field name in the {@link Grouper.property}. | ||
*/ | ||
public createGrouper(reverse: boolean): Grouper { | ||
return new Grouper(this.fieldNamePlural(), this.grouper(), reverse); | ||
return new Grouper(this.fieldNamePlural(), this.grouper(), reverse, this.comparator()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens with this code if the field does not implement sorting? I expect that calling If you look at the Quick Reference table you will find a few such cases - such as recurrence or recurring (I forget which). And certainly the new So... you could write a failing test using recurrence, but if it is taught to sort in future, that test will become useless. So the testing trick here is to write a new test and have it use a simple implementation for Field that does grouping but not sorting. Perhaps Then line 67 can be changed to only use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Very good point, thanks.
Yup, I did that now, this seems to be a good trade-off.
On There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think I got this, but not sure whether it is a good solution. Something smells here, can't tell what exactly. Probably the whole direction of the development ㅠㅠ |
||
} | ||
|
||
protected grouperRegExp(): RegExp { | ||
|
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.
I thing this will change (improve) the sort order for urgency grouping.
Which will just require confirmation and then a change the docs of group by urgency please.
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.
I checked the docs of urgency here and here but didn't find it related to the changes in this PR (Priority group names...).
Do you mean that now sorting of groups from urgency field can use this code? I agree with that, but I guess this should be a different PR.
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’s this text:
I am reasonably confident that this PR has done point 3 and the above text should be updated.
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.
To be more specific, this PR does several things:
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.
You are right! I wrote a test and tried it before and after all the changes and it does change the behaviour indeed. Test and docs.
Now I'm thinking that I should've probably split the PR in 2 - one for refactoring with addition of the comparator to the grouper and the other one to fix the Priority group names. May be a third for the Urgency =)
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.
Lmk if you wish to have a separate issue for the reversed urgency order =)