Combine Search and Dropdown component on LGV

[teresam856]

Issue1099
Before:

Progress so far:

TODO:

1. combine search and dropdown into the refNameAutoComplete component
2. Adjust component for future name indexing features
3. remove search bar component
4. options for searching adjusted
5. fix styling
6. fix tests/add tests
7. fix import form and add more tests

[teresam856]

[cmdcolin]

Those gifs look really great

[teresam856]

PR 1438
The animation of the circular progress may freeze at times for many contigs because of the extensive load. Just something to note. It is currently happening in master as well.

On another note: good future discussion would be if we want to keep the same behavior for the autocomplete component of the import form of the LGV and the autocomplete component of the header of the LGV. Only difference now is that locString navigation is disabled in the import form and only allowed once a ref sequence has been selected.

[cmdcolin]

I think probably any pause would not be due to PR #1438

That thread is simply a lot of refseqs, 3.5MB worth, and that processing has to be loaded to the main thread in the end anyways

Bottlenecks can be seen in the profiler. The setRegions call appears to take 5.7 seconds worth of main thread blocking time which would happen of whether we loaded the file on a webworker or not i think

Screenshot of profiler

[cmdcolin]

By that thread is a lot of refseqs I mean the "config_many_contigs.json" this loads a 3.5MB large.chrom.sizes file and the slowness is not in parsing it but hydrating it into a mst model I think

[cmdcolin]

Only difference now is that locString navigation is disabled in the import form and only allowed once a ref sequence has been selected.

Is there any reason for this? It seems like we should allow navigating to a locstring off the bat from the import form

[cmdcolin]

Note that the setRegions call that takes 5.7 on dev takes about 1s in production so thats a small comfort too

[cmdcolin]

One thing that is notable is that side scrolling the app on this branch with the config_many_contigs is quite slow (5-10fps consistently). There is also some slowness on master but it is less so (it is more stuttery, dropping down to 10fps every time it has to reupdate the header component likely from coarse dynamic blocks updates)

[teresam856]

Only difference now is that locString navigation is disabled in the import form and only allowed once a ref sequence has been selected.

Is there any reason for this? It seems like we should allow navigating to a locstring off the bat from the import form

Attempting to navigate from the import form results in a Type Error because the displayedRegions are not yet defined. To enable navigation with locStrings from the import form would require a separate implementation in order to parse the locString first to set the displayedRegions first and then attempt to navigate to the specific location. Not to say this is not possible, it is just that the flow of when things get set would need to change.

Other than that, I personally would not find it as intuitive to use locstrings there. The search aspect of the component would still work in filtering the options of ref names and identifiers that the user could choose from. But again, all this can modified.

[teresam856]

One thing that is notable is that side scrolling the app on this branch with the config_many_contigs is quite slow (5-10fps consistently). There is also some slowness on master but it is less so (it is more stuttery, dropping down to 10fps every time it has to reupdate the header component likely from coarse dynamic blocks updates)

Yeah I can see that it takes 148.4ms to recompute the array of options every time. --> I saw a significant improvement by using a useEffect to calculate the list of options. --> added useEffect to calculate the options (on last commit)

[rbuels]

I'm a little concerned with the use of useState and useEffect in this PR. We normally should only use these for very small localized state (like mouseover highlights), and keep everything else using mobx-state-tree views and actions.

Can you elaborate on the rationale behind using the React state management instead of our more project-standard mobx-state-tree?

[rbuels]

The reason for preferring mobx-state-tree for managing most state is because it helps us keep a little bit better separation between state, and the presentation of that state.

[cmdcolin]

I think the useState/useEffect in this case is for caching the value, but it might be able to be replaced with a MST getter, or could be changed to a useMemo to save the value

[cmdcolin]

#1509 (comment)

Regarding this, I think it would probably be good if we can make the locstring input work from input form. This could be spun out as a new issue if needed

[teresam856]

#1509 (comment)

Regarding this, I think it would probably be good if we can make the locstring input work from input form. This could be spun out as a new issue if needed

Now that I think about it, I think locStrings may be useful from the import form. Will create a new issue for it. Replaced the useEffect for useMemo for calculating the options, and removed unnecessary useEffect/useState.

[teresam856]

I'm a little concerned with the use of useState and useEffect in this PR. We normally should only use these for very small localized state (like mouseover highlights), and keep everything else using mobx-state-tree views and actions.

Can you elaborate on the rationale behind using the React state management instead of our more project-standard mobx-state-tree?

I was using useEffect as a potential place to fetch new options as the user's input changed (when considering future implementation of search by identifiers/name indexing system based on what i gathered from material ui autcomplete), but this can totally be implemented in mob x state tree with an action. Removed the useEffect/useState and replaced with useMemo for setting the options.

[rbuels]

I think I understand now. The new version of useMemo looks good, thanks for tweaking that. Merging!