Rework search components
Closes #1519 (closed)
Merge request reports
Activity
requested review from @babadie
assigned to @vrigal
added 1 commit
- 02b5bb43 - WIP: Remove query passed as modelValue and simplify search trigger
This was a real headache. I removed all the things that I found weird (and do not really understood to be honest).
At the end it is usable but there are still some errors:
-
Duplicated routewarnings and aCancelled routeerror when changing projects.- We could make this behavior simpler by searching in a single corpus at a time.
- I suppose the error must be patched before landing this.
- Page is not stored as a query parameter. This seemed complex to implement because it triggers reactivity of the query params watchers.
-
@mlbonhomme will try it today, and report nay findings (as user & reviewer)
@vrigal you'll need to rebase tommorow
-
One thing that caused me issues is the corpus selection (initally not set, then automatically triggering a search with default query parameters set in the URL). Yet I used a hack to avoid race conditions and keep the component mounted.
I'm testing if make it work with KeepAlive components, so we have one cached form per corpus. It should be easier to control mount (sets the URL with the default query, triggering search), restore URL when the component is activated (
onActivated), trigerring search too.One other thing is the page not stored as a query param (as its controlled by the children). IMO we could simply pass the page to the children as a prop, so it is set in the query params (& restore it).
added 19 commits
-
a7f2b0ce...61b2d9e6 - 6 commits from branch
release-1.7.1 - 61b2d9e6...f2cb0b47 - 3 earlier commits
- 7375e2a4 - Build a solution that works
- 662ca459 - Avoid cancelled route (duplicated) & add comments
- 7409ed90 - Improvements
- 521b2cca - Retrieve corpusId from .params
- 2884faba - Restore pagination
- 5fdef41b - Revert "Retrieve corpusId from .params"
- 59cc16c5 - Simplify management of search form per corpus
- 927d3216 - Prevent making an empty search
- 856abfc3 - Hide results when no source is selected
- 6d67d178 - Rebase
Toggle commit list-
a7f2b0ce...61b2d9e6 - 6 commits from branch
I tested the rebased version this morning and it's already really good.
There remains a slight bug upon source changes that are not updating the local state.
@erouchet will look into it in january. Thanks for all the work.
-
I spent more time on it today. Support for the page in query param was tricky but works now. I still have the issue with the non reactive property. After hitting F5, the correct filters are applied but almost nothing works anymore (happens even after removing the entire call to
init). The reactivity seems lost by VueJS, I must be missing something. Selecting another corpus fixes the issue.Note: it would be nice to lock the different facets while loading (ideally also sort by and page selection).
Edited by Valentin Rigal -
I worked on it with Erwan today, then still had to fix a bunch of things. Everything seems to be working now.
I noticed
src/components/Search/Result.vuecomponent redirects to the parent of the element with the highlight query param, which do not work when the parent is a folder. This could be a follow-up. changed milestone to %Arkindex 1.7.1
added 7 commits
- 9f57cf52 - Use an optional property for facets that can be unset
- aa077727 - Fix a VSCode type warning on search facets response
- 45f863e5 - Use a computed setter again for the corpus ID
- 9a0ba141 - Remove a type assertion and fix some TS errors
- bdb5c0ee - Remove routeQuery computed
- 9aebd057 - Use explicit typing
- bf31ff8d - Type the emitted events
Toggle commit list
added 2 commits
-
Me and @erouchet tried to handle route update (e.g. previous/next buttons), but the actual implementation do not allow to make this easily. Other points works. Erwan will try this approach today:
- The form uses a v-model (SearchQuery) instead of a custom event, it never access the route.
- The parent view handles route updates and triggers search.
Then we will discuss this again this afternoon.
added 33 commits
-
e653cc77...efc833f0 - 3 commits from branch
release-1.7.1 - efc833f0...5c952311 - 20 earlier commits
- 12bc5c49 - Fix a VSCode type warning on search facets response
- 14fcce76 - Use a computed setter again for the corpus ID
- 7b47d75c - Remove a type assertion and fix some TS errors
- 224fa3f0 - Remove routeQuery computed
- d1d2b331 - Use explicit typing
- e8e97577 - Type the emitted events
- a14adae9 - Initially trigger search
- 7ec8a36a - Prevent triggering search with empty query
- 3d53db95 - Fix init with similar query terms
- 2d93e4c2 - Start entirely from scratch
Toggle commit list-
e653cc77...efc833f0 - 3 commits from branch
added 7 commits
- 554ed187 - Fix typing issues
- 8565d97b - Trigger full searches when loading the page
- 6722b947 - Use the correct corpus ID when switching corpora
- 947ff5a3 - Add page number to SearchQuery type
- d9f5eac0 - Properly handle query changes, especially when switching corpora
- 77633b0c - Overwrite checked facets when they are updated in the URL
- 585fa22a - Handle selecting and unselecting a corpus
Toggle commit list-
I started from scratch and added a few of the typing fixes we wrote before, for example on the handling of
MetaDataType. Then I did some tests and found that most of the route handling already worked. The browser previous/next buttons do work when editing a query in the same project, but not when switching to another project, and page numbers were not always preserved, along with some other edge cases.I removed the
only_facets=truesearch. The original design for this search page was to only fetch facets and not show any result until an actual search was made, but now we want the search to run all the time, so it no longer made sense. This made all search queries load when reloading the page, and also got page numbers to work.I then worked on other edge cases like switching projects and using the browser prev/next buttons. Most of the work was in handling the currently selected facets and updating them when switching projects or when the available facets changed.
@vrigal will now test with his own knowledge of the many ways in which one can try to break this page
-
I think it is the good approach, it works fine indeed. I noticed a few things that I will try to fix now (in order of importance):
- The entire form must be reset when switching project (as we manually set
query=*, this is already done for facets and terms). -
sortquery param should be restored in the form from initial query params (search request is fine though). - Fix loading attributes (done in !1817 (ef18b271), prevents race conditions).
- It is kind of a hack, but we can try to reuse commit !1817 (3e5c4aea) to hide results when no source is selected (otherwise it is quite confusing). It should be easy.
- I got duplicated route when clicking on search and no request is sent (this could be done via event, maybe this is not necessary). If a page is set returns to page 1.
Edited by Valentin Rigal - The entire form must be reset when switching project (as we manually set
added 6 commits
- e102f099 - Completely reset form when switching project
- a0057eba - Restore sort from the query parameters (initial and router navigation)
- 3b191cac - Prevent duplicating search when sort value is restored
- 02f5a733 - Disable all fields while loading
- 888c7b6f - Fix query comparison to trigger new search
- 346524a1 - Hide results when no source is selected
Toggle commit listrequested review from @erouchet
