Go cam viz #823
Go cam viz #823
Conversation
|
I've fixed the lazy loading/duplicate tabs issues and also pushed as go ribbon and go-cam sharing tabs for now. The tabs changes are all in one commit so easy to revert if needed. |
src/uniprotkb/components/protein-data-views/styles/go-cam-viz.module.scss
Outdated
Show resolved
Hide resolved
| useEffect(() => { | ||
| goCamVizLoader.defineCustomElements(); | ||
| customElements.whenDefined('wc-gocam-viz').then(() => { | ||
| setDefined(true); |
There was a problem hiding this comment.
Async state-setting logic within a useEffect is something to watch out for. Their could be the case of the promise resolving after the component has unmounted, and then it'll try to set a state for a component that doesn't exist anymore.
Maybe use useSafeState instead of useState.
Have a look at the logic in useCustomElement, which internally uses useSafeState to do similar things that you're doing here. useSafeState is basically a wrapper to make sure that there is no set state when the component is unmounted.
| async function fetchGoCamModels() { | ||
| if (goCamIdToItem.size) { | ||
| const mapper = (id: string) => | ||
| fetchData<GoCamModelInfo>( | ||
| externalUrls.GeneOntologyModelInfo(id) | ||
| ).then((response) => ({ | ||
| id, | ||
| data: response.data, | ||
| })); | ||
| const results = await pMap(Array.from(goCamIdToItem.keys()), mapper, { | ||
| concurrency: heuristic.concurrency, | ||
| }); | ||
| setUniprotGoCamIds( | ||
| results | ||
| .filter(({ data }) => isUniprotCurated(data)) | ||
| .map(({ id }) => id) | ||
| ); | ||
| } | ||
| } | ||
| fetchGoCamModels(); |
There was a problem hiding this comment.
Same comment as in GoCamViz.tsx regarding state setting in async logic. use useSafeState.
Also use one single AbortController both for each fetch and for p-map (look for signal in the documentation https://www.npmjs.com/package/p-map ). Look in the codebase for controller.abort() to see example usage.
This will also to stop current and subsequent fetches if the user changes page or switch the tab back
|
Playing with the UI I've just realised that most of the content in the panel on the right is links, and most of those links are to other websites. I don't really like that we have a fairly big section of UI that behaves completely differently than the rest of the website Mouse cursor but no UI indicator of a link make the user think it might be a button with an action on the current page. It would be better that all link use the bold/blue style that we use for links elsewhere. That would probably require a change of style for the clicked section otherwise the links would blend into the selected background (change background colour? Just have a border to show selection?). Also, external links not marked as such is a big problem, especially when some of the concepts links do already exist in UniProt (e.g. "part of" or "occurs in" values that look like they could be UniProt subcell locations or keywords). I'm not entirely sure what is doable or not though, given that it's all within custom elements that will be a bit more complicated to style. |
| const cancelTokenSource = axios.CancelToken.source(); | ||
| const abortController = new AbortController(); | ||
| const { signal } = abortController; | ||
| signal.addEventListener('abort', () => { |
There was a problem hiding this comment.
I was wondering why that was needed, but I see the axios cancel token logic above. It's one of these cases where using plain fetch would be easier as AbortController fits well into the native fetch logic.
No need to change here, just a comment from me now
Note
wc-gocam-vizreleased which addresses many issues.Purpose
Add wc-gocam-viz to uniprotkb entry page.
Approach
TreeSelectfor user selection of go-cam idTreeSelectfor Complex Viewer uiTesting
Updated existing test.
Checklist