-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Issue #1957]: Default sortby to posted date descending #1970
Conversation
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🟢 | Statements | 84.17% (+0.03% 🔼) |
872/1036 |
🟡 | Branches | 65.12% (+0.1% 🔼) |
224/344 |
🟡 | Functions | 75.58% | 164/217 |
🟢 | Lines | 84.22% (+0.03% 🔼) |
811/963 |
Test suite run success
164 tests passing in 56 suites.
Report generated by 🧪jest coverage report action from 0f33630
{ label: "Posted Date (Ascending)", value: "postedDateAsc" }, | ||
{ label: "Posted Date (Descending)", value: "postedDateDesc" }, | ||
{ label: "Close Date (Ascending)", value: "closeDateAsc" }, | ||
{ label: "Agency (Ascending)", value: "agencyAsc" }, |
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.
@andycochran fyi the new order
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.
all descending variants come before ascending now
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.
🤔 IDK if Z–A should come before A–Z. The date ones should be descending first. But the others makes more sense as ascending first. Can we also change them to:
"Posted Date (newest)" // descending
"Posted Date (oldest)"
Close Date (upcoming) // descending
Close Date (oldest)
Opportunity Title (A to Z) // ascending
Opportunity Title (Z to A)
Agency (A to Z)// ascending
Agency (Z to A)
Opportunity Number (ascending)
Opportunity Number (descending)
@crystabelrangel, do you have any input on ^these labels?
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.
What about for posted date something like Posted Date (newest/descending)
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.
These look good to me!
Some thoughts/questions:
What if we keep Close Date consistent with Posted date. Once something is Closed, it's closed. So it wouldn't be upcoming, right?
Close Date (newest) // descending
Close Date (oldest)
And Opportunity Number is confusing. How are those even being sorted? They have numbers and letters. I know it's what is on live, so might be good to get some analytic info before making a choice if we should keep it.
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.
However, the results set might not necessarily have any results with close dates in the future. Maybe just newest/oldest is fine, and we ignore the fact that "newest" may be in the future.
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 should track (via google analytics) if anyone ever even sorts by opp number. If not, or if it's an edge case, we might remove this sort option. I'm not too worried about how we label that one.
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.
Let's use:
Posted Date (newest)
Posted Date (oldest)
Close Date (newest)
Close Date (oldest)
Opportunity Title (A to Z)
Opportunity Title (Z to A)
Agency (A to Z)
Agency (Z to A)
Opportunity Number (ascending)
Opportunity Number (descending)
^ in that order
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.
thanks folks - will try to get to this soon!
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.
Summary
Fixes #1957
Time to review: 5 min
Changes proposed
Screen.Recording.2024-05-08.at.7.58.34.PM.mov