improve search #749
improve search #749
Conversation
| @@ -45,5 +45,12 @@ | |||
| {% endblock %} | |||
|
|
|||
| {% block js %} | |||
| <script nonce="{{ request.csp_nonce }}" > | |||
| {% if language_facets %} | |||
There was a problem hiding this comment.
can this be passed in as a data attribute on the sidebar div instead? theres a function extractDataParams to help with this on the js side
There was a problem hiding this comment.
you mean without attaching language_facets to window global variable?
There was a problem hiding this comment.
yeah like
<div id="sidebar" {% if language_facets %} data-language-facets="{{ language_facets }}" {% endif %} />mostly for consistency, not sure if there's any meaningful practical difference
There was a problem hiding this comment.
In what file would you do this? As I understand this, we'd need to do this in jinja, but we need the data in the vue component... I am lost here.. help, please!
There was a problem hiding this comment.
this one, the js side parses it with extractDataParams before mounting the component and passing the extracted stuff as props. Most mounting scripts in /apps do it
There was a problem hiding this comment.
oh, got it! thanks!
There was a problem hiding this comment.
thanks @asuworks this looks great! still need to test & deploy locally but some minor coding style recommendations.
I think we should try to keep our logging formats consistent throughout the codebase but the format strings are definitely easier to read with lots of parameters...
django/core/jinja2/common.jinja
Outdated
| placeholder="{{ placeholder }}"> | ||
| <button type="submit" class="btn btn-primary"> | ||
| <i class="fas fa-search px-1"></i> | ||
| </button> | ||
| </div> | ||
| {% if query_params %} | ||
| {% set params = query_params.split('&') %} |
There was a problem hiding this comment.
think we can move this param splitting logic out of the template and into Python? Perhaps as a method in jinja globals that takes search params and generates a set of name/value hidden input pairs.
django/core/pagination.py
Outdated
| if key == "tags": | ||
| elif key == "ordering": | ||
| for v in values: | ||
| if v == "": |
There was a problem hiding this comment.
should this first condition be an else clause fallback instead?
django/core/pagination.py
Outdated
| filters.append("Sort by: Publish date: oldest") | ||
| elif v == "-last_modified": | ||
| filters.append("Sort by: Recently Modified") | ||
| elif key == "tags": | ||
| filters.extend(tag for tag in values) |
There was a problem hiding this comment.
isn't this the same as filters.extend(values)
django/core/pagination.py
Outdated
| for v in values: | ||
| if v == "": | ||
| filters.append("Sort by: Relevance") | ||
| elif v == "-first_published_at": |
There was a problem hiding this comment.
this conditional mapping might be better organized as a defaultdict data structure like
from collections import defaultdict
SORT_BY_FILTERS = defaultdict(
lambda v: return "Sort by: Relevance",
{
"-first_published_at": "Sort by: Publish date: newest",
"first_published_at": "Sort by: Publish date: oldest",
...
}
)
Then this turns into filters.append(SORT_BY_FILTERS[v])
django/core/pagination.py
Outdated
| Args: | ||
| query: request query | ||
| data: results from Elasticsearch | ||
| current_page_number: request parameter |
There was a problem hiding this comment.
args docstring should be renamed to page as well? also not sure what request parameter means, is this the integer page value from the query params?
django/core/view_helpers.py
Outdated
| if criteria: | ||
| logger.debug(f"criteria={criteria}") |
There was a problem hiding this comment.
This is a very minor note but for codebase consistency make sure logger statements are of the form
logger.debug("criteria=%s", criteria)
See https://pylint.readthedocs.io/en/stable/user_guide/messages/warning/logging-fstring-interpolation.html for more context but there's also this rabbit hole pylint-dev/pylint#2395
django/core/view_helpers.py
Outdated
| except FieldError: | ||
| logger.warning("Invalid filter criteria: %s", criteria) | ||
| except FieldError as e: | ||
| logger.warning(f"Invalid filter criteria: {criteria}. Error: {str(e)}") |
There was a problem hiding this comment.
use exc_info=e instead of converting the exception into a string and missing out on the entire stack trace
| @@ -3,10 +3,12 @@ import { useAxios, type RequestOptions } from "@/composables/api"; | |||
|
|
|||
| interface CodebaseQueryParams { | |||
| query?: string; | |||
| publishedAfter?: string; | |||
| publishedBefore?: string; | |||
| published_after?: string; | |||
There was a problem hiding this comment.
is this to line up with the incoming query parameters? otherwise camelCase would be preferred
There was a problem hiding this comment.
Nice. Filter sidebar looks great and the search querying anecdotally seems a lot better
The default query should probably go back to ordering things by publish date descending though
I like the new selected filter terms display better than the one that comes from the paginator macro at the top. In the case of codebases should we disable those and just use the sidebar ones so we aren't showing that information in two places? If so it could:
- get moved to the top of the sidebar
- only show up when those filters are applied
- have a button to clear all
thanks @alee! fixed it! |
|
|
the issue I have is that the whole sidebar is like 1000px tall without filters so you have to scroll up and down to do stuff, maybe it can be shrunk down to something more manageable? If the selected filters only appear when applied (which I think is what should happen) it may be less of an issue since the page would reload anyways. Could also just apply on selection, I don't think we have nearly enough different filters to make that a problem, even without AJAX searching |
|
how about programming languages in 2 columns?
|
|
we could also remove the "Publish a model" button (I think it is not very necessary there) |
|
the publish button should be somewhere on that page, maybe gets moved to whatever we replace that blue alert box with. The counts wrapping over looks a bit odd, what about collapsing it by default? Or showing just a few with a Other stuff could probably be shrunk down too, padding, margins, text size, etc. The publish date filters can be together in one row |
|
|
|
much nicer and shorter :) |
|
the wrapping is not nice, agree... maybe back to single column? The collapsing of the programming languages would complicate the ui I think... |
|
pretty sure thats the only link to publishing a model, I wouldn't want to suddenly move it. Regardless it still hardly fits on my screen How does this UI look? I squished things down a bit and removed the apply button assuming we can apply everything on selection. If not, then the selected filters can sit at the bottom with the apply button since most screens should have the full thing in view. There should at least be a way to clear them from the top though
|
|
@sgfost I implemented your version. When selecting a checkbox or a radio in the sidebar, the ui jumps down because the selected filters are populated at the top... not so nice... maybe put them back at the bottom? |
|
I had in mind only displaying them when they are actually applied. I suppose that may still pose an issue if things are applied on selection.. Its probably fine at the bottom as long as the whole thing isn't too tall |
|
Today we talked about showing selected filter badges in the sidebar on page load... |
alee
force-pushed
the
21-search-improvements
branch
from
b155636 to
b5cd979
Compare
alee
force-pushed
the
21-search-improvements
branch
from
b5cd979 to
42c2760
Compare
|
@all-contributors please add @sgfost for doc, review, maintenance, mentoring |
|
This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 2854 |
alee
force-pushed
the
21-search-improvements
branch
from
42c2760 to
096bce2
Compare
|
@all-contributors please add @asuworks for code, doc |
|
This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 2854 |
alee
force-pushed
the
21-search-improvements
branch
from
096bce2 to
52da007
Compare
alee
force-pushed
the
21-search-improvements
branch
4 times, most recently
from
1c053ac to
4a18fb0
Compare
set `max_result_window` and guard bad `page` parameter requests for /codebases/ and /search/
add programming languages and better search queries for codebases
- enhance elasticsearch query - remove excessive logging
- use data attribute to pass props to vue component instead of attaching to the global window object - minor refactoring and cleanup - set initial ordering of codebases to published date: newest - add stopwords for codebase library search - rearrange codebase filtering sidebar ui
1. On initial page load, codebases are sorted by date: newest. 2. On first search, results are sorted by relevance 3. Disable buttons in the sidebar when loading 4. Disable "Apply filters" button if filters are unchanged
- add basic test for codebase search - use lodash instead of rolling our own deep equality function, https://lodash.com/docs#isEqual - add programming language facets to query string directly if selected - could also switch back to @asuwork's original implementation that generated ran a second query, e.g., ``` codebases = Codebase.objects.filter(releases__programming_languages__name__in=programming_languages) criteria.update(id__in=codebases.values_list('pk', flat=True)) ``` to avoid filter on related fields limitations in wagtailsearch ``` Cannot filter search results with field "name". Please add index.FilterField('name') to Codebase.search_fields. ```
alee
force-pushed
the
21-search-improvements
branch
from
4a18fb0 to
c20ee16
Compare
trying to close https://github.com/comses/planning/issues/21