Allow multiple searches in the state #93
Conversation
this action is responsible to create (if not exists) a new entry in the state dictionnary for the dedicated search, action.id is the new search key
It's responsible to interact with just a slice of the state: the search matching its searchId The facade must be init with a searchId, all observables are created during the init, and not during the instanciation because we need to know the searchId. This way, using the facade will be transparent for other services and component. This service is not instanciated at Root level and is not meant to be a singleton, we MUST implicitly inject and instanciate this service for each searches
…chFacade the facade will be used by all components under the DOM node where the directive is applied
and never use the store directly. Observable based on the state selector must be init during onInit() phase
This implies to create a new component that embed all the search elements. The directive must be applied outside of the directive otherwise the SearchFacade service is no correctly injected
the facade ist inject in root module abstract component class handle the init of the facade, I didn't want to use the directive cause it implies to create a sub component
… can trigger a search at the same time. We should implement our own observable to manage this.
| response, | ||
| this.searchService.configuration.basePath | ||
| switchMap((action: SearchActions) => | ||
| this.authService.authReady().pipe( |
There was a problem hiding this comment.
Is it intentional to pipe all the work inside the authReady stream ? Why not go back to the actions$ stream like before ?
|
I'm a little confused with the name 'getSearchStateSearch'. Will there be anything else than searches in the state ? And if it's to recognize that we only get one search in the dictionary, why not getSearchById or getSearchByKey ? |
Yes good idea, I used the common pattern but it's not appropriate here, thanks. Beside that, what do you think of the global approach ? |
|
It's really nice that you tried to stay as close as possible to the previous state. Now that I think about it, managing the state as ngrx entities would have been a step too far. |
This PR aims to change the search state structure to handle multiple searches in the same page.
Before, the state structure allowed to store a unique search, matching the
SearchSateinterface. This structure has been enhanced so the state stores a dictionnary of searches, theSearchSatehas become a dictionnary ofSearchSateSearch.The dictionnary key is the search id, used to bind any component and action to a specific search, the default key is
default.I tried to keep the code simple so the change is quite transparent:
defaultvalueBy default, the
initialStatecontains a search state for thedefaultid, and all actions work on this id in a transparent way.At the moment, the application looks to run exactly the same.
State Facade
searchSearchStateContainer="mainSearch"directivesearchIdsearchId, the default value isdefault.I had to refactor many components and tests that were using the store directly.
I don't guarantee everything is working perfectly but it looks ok with the tests I've made.
Demo: https://geonetwork.github.io/geonetwork-ui/multiple_search/demo/webcomponents/gn-results-list-multiple.sample.html
The commit 1057df2 should be adressed (usage of
flatMapoperator).