-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix: set initial ordering to relevance
in the frontend component
#766
Conversation
relevance
in the frontend componentrelevance
in the frontend component
relevance
in the frontend componentrelevance
in the frontend component
@@ -141,7 +141,7 @@ const initializeFilterValues = () => { | |||
values.endDate = urlParams.get("publishedBefore") | |||
? new Date(urlParams.get("publishedBefore")!) | |||
: null; | |||
values.ordering = urlParams.get("ordering") || "-first_published_at"; | |||
values.ordering = urlParams.get("ordering") || "relevance"; |
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.
Can you explain what behavior this is attempting to fix? The initial sort order should be newest to oldest as "relevance" doesn't make sense on initial load; won't this set the initial sort order to "relevance" when the ordering query param isn't explicitly set?
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.
If the user sets the ordering explicitely, the ordering will be available through urlParams.get("ordering")
. If not, the default ordering during searching (seqrch query is not empty) is "relevance".
But I guess, you also have a point, namely: when there is no search query, just filtering by programming languages. In which case it should be something like:
values.ordering = urlParams.get("ordering") || (urlParams.get("query") ? "relevance" : "-first_published_at");
…to -first_publiched_at
change to "clear search" and include for queries as well we should not list a sort by relevance when there is no search query
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 didn't see an easy way to hide "Relevance" based on the way ListSortBy.vue and codebase_list.ts are currently set up but if you can think of that would be ideal
Here is a quick fix to remove |
looks good to me, go ahead and submit a new PR for it and we'll get it merged and deployed |
No description provided.