NickAkhmetov / HMP-235 Add support for text-only publication vignettes #3159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lchoy
reviewed
Jul 11, 2023
Co-authored-by: L Choy <[email protected]>
john-conroy
reviewed
Jul 11, 2023
context/app/static/js/components/publications/PublicationVignette/hooks.ts
Outdated
Show resolved
Hide resolved
context/app/static/js/components/publications/PublicationVignette/hooks.ts
Outdated
Show resolved
Hide resolved
- Account for potentially undefined/null vignettes (no guarantee that it'll be an empty list) - Move multiFetcher to shared utils folder
john-conroy
reviewed
Jul 12, 2023
Comment on lines
16
to
23
| const urls = | ||
| vignette.figures?.map( | ||
| ({ file }) => `${assetsEndpoint}/${uuid}/vignettes/${vignetteDirName}/${file}?token=${groupsToken}`, | ||
| ) ?? []; | ||
| const { data } = useSWR(urls, multiFetcher, { | ||
| revalidateOnFocus: false, | ||
| revalidateOnReconnect: false, | ||
| }); |
There was a problem hiding this comment.
What happens when figures is undefined and in turn urls are undefined?
There was a problem hiding this comment.
The nullish coalescing fallback on line 19 ensures that urls is always at least an empty list; even if that wasn't there, swr's conditional fetching functionality would ensure that the fetch doesn't run, data remains undefined, and the return undefined case on line 42 would be reached. https://swr.vercel.app/docs/conditional-fetching
There was a problem hiding this comment.
Ah sorry didn't catch that!
There was a problem hiding this comment.
Actually, on second thought, that ?? [] shouldn't be there since an empty array is truthy. One moment.
john-conroy
previously approved these changes
Jul 12, 2023
john-conroy
approved these changes
Jul 12, 2023
NickAkhmetov
deleted the
nickakhmetov/hmp-235-enable-display-of-text-only-vignettes
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adjusts the Vitessce vignette rendering and fetching logic:
js/*paths are properly recognized in TS filesI've tested this locally; if there are no conf files provided, the accordion only displays the markdown description.
The markdown description is also instantly visible as the accordion expands/Vitessce loads, instead of popping in at the same time Vitessce finishes loading. You can see it flash by in the recorded demo:
2023-07-10.hmp-235.demo.mov
Future Improvements
I ran into some problems with Storybook failing to parse
Contexts.tsxafter I tried to convert that file; I'll revisit it at another time (created HMP-250), and have worked around it by defining the types in JS comments.