Uh oh!
There was an error while loading. Please reload this page.
#66 🪟 Add Visualizer Modal - #248
Conversation
| const isBamFile = Boolean(file_type && BamFileExtensions.includes(file_type)); | ||
| return idMatch && isBamFile; | ||
| }); | ||
| const fileData = tableData.filter(rowIsFileData) as FileTableData[]; |
There was a problem hiding this comment.
tableData is unknown[] | [] here so type guard isn't working as expected without casting. Open to suggestions.
There was a problem hiding this comment.
I'm wondering if this is a consequence of using an older version of TS, since I believe it should properly update the type on the latest TS versions. The filter is protecting things well so this is fine enough.
There was a problem hiding this comment.
Also can't this just be derived state instead of additional useEffect?
joneubank
left a comment
There was a problem hiding this comment.
Some notes on the Visualizer Button interface.
Uh oh!
There was an error while loading. Please reload this page.
| isBamFileSelected: boolean; | ||
| isFileTableActive: boolean; | ||
| switchTable: (t: string) => void; | ||
| openModal: () => void; |
There was a problem hiding this comment.
openModal is required property but is not used if visualizersEnabled is false... so now we require someone using this header button to provide a openModal value even if it is not used.
We should make sure we don't require unused props, can use discriminated union as our props based on visualizersEnabled, or move the visualizers properties to an optional visualizersConfig prop and then make that config prop's existence be used to enable/disable the button.
There was a problem hiding this comment.
You have a good point about using visualizersConfig; the two boolean props are ONLY used for disabled={!visualizersEnabled && isFileTableActive} so I'm going to combine them as a disabled prop
And then openModal can be made optional as well
| } | ||
| `} | ||
| onClick={() => switchTable(tableTypes['BAM_TABLE'])} | ||
| onClick={openModal} |
There was a problem hiding this comment.
With this change, all of the behaviour of this button has been moved outside of this component. The button does whatever openModal specifies. Even a decision about whether it is enabled is based on props.
Perhaps its just a case where we need to rename the props to onClick instead of openModal and replace visualizerEnabled and isFileTableActive to a single disabled or enabled prop.
There was a problem hiding this comment.
I will look into how I can organize this better. The intent was to have openModal / closeModal as fixed utilities which always do the same thing, but because they use state you can't export them that way, and have to be scoped within a component render/props pattern (which is intended to be mutable). So you get this sort of organizational conflict.
| const isBamFile = Boolean(file_type && BamFileExtensions.includes(file_type)); | ||
| return idMatch && isBamFile; | ||
| }); | ||
| const fileData = tableData.filter(rowIsFileData) as FileTableData[]; |
There was a problem hiding this comment.
I'm wondering if this is a consequence of using an older version of TS, since I believe it should properly update the type on the latest TS versions. The filter is protecting things well so this is fine enough.
There was a problem hiding this comment.
Couple of comments. Unclear what level of "correct" we're going for here based on the future of "stage", so I'll leave that decision (pr approval) up to the architects 👍 (I think @joneubank has eyes on this)
The overall page content component feels like it's doing too much, and some of this code could be placed into it's own component and there's a lot of mixed up styling paradigms.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| const getCardContainerStyle = (theme: Theme) => css` | ||
| display: flex; | ||
| height: 100%; | ||
| .file-container { | ||
| bottom: 10px; | ||
| position: absolute; | ||
| .badge { | ||
| display: inline-flex; | ||
| border: none; | ||
| border-radius: 20px; | ||
| margin: 5px; | ||
| min-width: fit-content; | ||
| padding: 3px 10px; | ||
| background-color: ${theme.colors.accent}; | ||
| color: ${theme.colors.white}; | ||
| &.disabled { | ||
| background-color: ${theme.colors.grey_6}; | ||
| } | ||
| &.format { | ||
| background-color: ${theme.colors.accent_light}; | ||
| &.disabled { | ||
| background-color: ${theme.colors.grey_5}; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| .visualizer-card { | ||
| background: unset; | ||
| border: 1px solid ${theme.colors.grey_5}; | ||
| border-radius: 16px; | ||
| cursor: pointer; | ||
| display: inline-flex; | ||
| font-family: 'Lato', sans-serif; | ||
| margin: 0 0.5rem; | ||
| padding: 10px; | ||
| position: relative; | ||
| width: 33%; | ||
| :disabled { | ||
| cursor: not-allowed; | ||
| } | ||
| p { | ||
| font-weight: 400; | ||
| font-size: 16px; | ||
| line-height: 16px; | ||
| margin-top: 0px; | ||
| text-align: left; | ||
| height: 30%; | ||
| } | ||
| h4 { | ||
| display: inline-block; | ||
| font-size: 18px; | ||
| margin: 0.5rem; | ||
| } | ||
| h5 { | ||
| font-weight: 700; | ||
| font-size: 16px; | ||
| margin: 0.25rem 0; | ||
| text-align: left; | ||
| } | ||
| } | ||
| .logo { | ||
| height: 18px; | ||
| vertical-align: text-bottom; | ||
| width: 18px; | ||
| } | ||
| .preview { | ||
| max-height: 28%; | ||
| overflow-y: hidden; | ||
| width: 100%; | ||
| img { | ||
| width: 100%; | ||
| } | ||
| } | ||
| `; | ||
There was a problem hiding this comment.
intermixing classic CSS classname styling with CSS-in-JS code style established in the codebase.
There was a problem hiding this comment.
OK I've moved use of classes into a CSS-in-JS pattern
| <div className={`badge ${isJbrowseEnabled ? '' : 'disabled'}`}>5 Max</div> | ||
| <div className={`badge format ${isJbrowseEnabled ? '' : 'disabled'}`}>.VCF</div> | ||
| <div className={`badge format ${isJbrowseEnabled ? '' : 'disabled'}`}>.BAM</div> |
There was a problem hiding this comment.
I'm guessing this has something to do with usage of ReactModal.... but using the established CSS patterns, in this case Emotion CSS-in-JS, is usually a workable solution. Emotion specifically works with the standard className prop.
| style={css` | ||
| vertical-align: middle; | ||
| `} |
There was a problem hiding this comment.
I don't think this works - css call from Emotion needs the className prop or the css prop, as style works completely differently
There was a problem hiding this comment.
This does work correctly, you can inspect the Modal X icon in browser and edit the vertical-align property
There was a problem hiding this comment.
Ah I see, this is confusing.
style is already a html attribute and we're overloading the meaning in Dismiss component.
So it's a custom prop called 'style' that then uses the Emotion css prop internally.
So we have both className and style in use ... sometimes they're html attributes.. other times they're React props, and sometimes they're strings and styled as selectors, and sometimes they're styled using Emotion functions.
There was a problem hiding this comment.
Yeah the Icon components are all using this style prop convention
Uh oh!
There was an error while loading. Please reload this page.
| useEffect(() => { | ||
| if (firstRender) { | ||
| currentFilters && setSQON(currentFilters); | ||
| ReactModal.setAppElement('#pageContent'); | ||
| setFirstRender(false); | ||
| } | ||
| }, [currentFilters, firstRender, setSQON]); |
There was a problem hiding this comment.
I don't understand the firstRender logic here. Usually a code smell having state to check the first render.
Shouldn't this just be on mount?
then this:currentFilters && setSQON(currentFilters);
is manually syncing syncing state between them when they should share a single source of state?
There was a problem hiding this comment.
doesn't look like this actually using url state either? Nothing updates in the url bar on filter selection.
I thought Stage was using all Overture work, where as the "currentFilter" stuff is ARGO code.
Basically the thing I mentioned in the demo about everything reading from sqon state
There was a problem hiding this comment.
I did not create the firstRender useEffect here, you'd have to ask @justincorrigible, but I am leveraging it here because ReactModal crashes if the targeted app element isn't rendered on the page.
So useEffect manages Next/Arranger/ReactModal setup here, other approaches weren't working for me.
re: fileData derived state, that is a good call, useEffect was added for early dev states setting up the Iobio Components but is no longer required
There was a problem hiding this comment.
I see. Sounds like a "onMount" using useEffect.
You may not have added "firstRender" but I don't understand why setting ReactModal.setAppElement('#pageContent'); depends on "firstRender" state
There was a problem hiding this comment.
Not explaining the need for on mount behaviour... but the linter will require the firstRender in the dependency array because its used in the function. Leads to some strange behaviour where setting firstRender to true will repeat the effect.
| const isBamFile = Boolean(file_type && BamFileExtensions.includes(file_type)); | ||
| return idMatch && isBamFile; | ||
| }); | ||
| const fileData = tableData.filter(rowIsFileData) as FileTableData[]; |
There was a problem hiding this comment.
Also can't this just be derived state instead of additional useEffect?
| <div | ||
| css={css` | ||
| width: 100%; | ||
| `} | ||
| > |
There was a problem hiding this comment.
This feels hacky, div should be full width by default, it's a fundamental diff between div and span blocks.
I removed it and on a quick glance everything looks ok, but please verify
There was a problem hiding this comment.
Yes this is definitely overkill, removed + a few more instances of width: 100%. CSS is where the hacking can go into overdrive some times ;)
ciaranschutte
left a comment
There was a problem hiding this comment.
curious about the use of css unset vs none?
I had a second swing at some of my wordy suggestions. Obviously this is a quick pass, and ignoring css stuff mostly. Just trying to align with React a bit better - making a component hierarchy, limiting interfaces for components, correct lifecycle usage (render => useEffect => render?). needless to say this should also be put into seperate files/better naming etc and it's not fully complete eg. badges needs more work. typeVizDetailProps={title: string,description: string,previewImage: string,logoImage: string,};constVizDetail=({
title,
description,
previewImage,
logoImage,}: VizDetailProps)=>{return(<><divcss={css`max-height:28%;overflow-y: hidden;img {width:100%; } `}><imgsrc={previewImage}/></div><divcss={css`text-align: left; `}><imgcss={css`height:18px;vertical-align: text-bottom;width:18px; `}src={logoImage}/><h4css={css`font-size:18px;margin:0.5rem; `}>{title}</h4></div><pcss={css`font-weight:400;font-size:16px;line-height:16px;margin-top:0px;text-align: left;height:30%; `}>{description}</p></>);};constBadges=({ isEnabled, badges })=>{returnbadges.map((badge)=>{return<divcss={badgeStyle({isDisabled: !isEnabled})}>{badge}</div>;});};exportconstVizOption=({
onClick,
isEnabled,
viz,}: {onClick: ()=>void,isEnabled: boolean,viz: VizDetailProps,})=>{return(<buttoncss={css`background: unset;border:1px solid ${theme.colors.grey_5};border-radius:16px;cursor: pointer;display: inline-flex;margin:00.5rem;padding:10px;position: relative;:disabled {cursor: not-allowed; } `}disabled={!isEnabled}onClick={onClick}><div><VizDetailviz={viz}/><divcss={css`bottom:10px; `}><h5css={css`font-weight:700;font-size:16px;margin:0.25rem0; `}>
Files:
</h5><divcss={css`display: flex; `}><BadgesisEnabled={isEnabled}badges={viz.badges}/></div></div></div></button>);};// Encapsulates the 3rd party component, some static, and if modal is openconstVizModal=({ children, appRootId, isModalOpen })=>{useEffect(()=>{ReactModal.setAppElement(appRootId);},[]);return(<ReactModal/** * removed ariaHideApp={!!firstRender} * it's defined in its type as "Boolean indicating if the appElement should be hidden. Defaults to true." * so this is only setting to hide app (which is set elsewhere) when firstRender is true? */isOpen={isModalOpen}style={{overlay: {zIndex: 10,backgroundColor: "rgba(0,0,0,0.7)",},content: {top: "15%",left: "20%",width: "60%",height: "70%",padding: "12px",},}}><divcss={css`display: flex;flex-direction: column;font-family:"Lato", sans-serif;height:100%; `}><divcss={css`display: flex;justify-content: end; `}><buttoncss={css`background: unset;border: unset;cursor: pointer;:hover {svg {path {fill:${theme.colors.grey_5}; } } } `}onClick={closeModal}><Dismissstyle={css`vertical-align: middle; `}height={12}width={12}fill={theme.colors.accent_dark}/></button></div><h3css={css`font-size:24px;font-weight:700;line-height:38px;margin:00.5rem; `}>
Select Visualization App:
</h3><pcss={css`font-size:16px;font-weight:400;line-height:26px;margin:0.5rem; `}>
Choose the appropriate app to analyze your selected data.
</p><divcss={css`display: flex;height:100%; `}>{children}</div></div></ReactModal>);};/** * * Responsibility is rendering the options available * ie. read configs, gather data etc then render each option with data * */exportconstVisualizerModal=({
closeModal,
setTable,
isModalOpen,
currentFiles,
appRootId,})=>{const{NEXT_PUBLIC_BASE_PATH,NEXT_PUBLIC_IOBIO_ENABLED,NEXT_PUBLIC_JBROWSE_ENABLED,NEXT_PUBLIC_CBIOPORTAL_ENABLED,}=getConfig();constisJbrowseEnabled=NEXT_PUBLIC_JBROWSE_ENABLED&¤tFiles.length<=5;constisCBioEnabled=NEXT_PUBLIC_CBIOPORTAL_ENABLED&¤tFiles.length<=2;constisIobioEnabled=NEXT_PUBLIC_IOBIO_ENABLED&¤tFiles.length===1&¤tFiles[0].file_type&&BamFileExtensions.includes(currentFiles[0].file_type);// onclick always 1) sets a table option an 2) close modal, we can encapsulate all this in a single functionconstselectViz=(tableType)=>()=>{setTable(tableType);closeModal();};return(<VizModalisModalOpen={isModalOpen}appRootId={appRootId}><VizOptiononClick={selectViz(tableTypes.JBROWSE_TABLE)}isEnabled={isJbrowseEnabled}viz={{title: "JBrowse",description:
"A fully featured genome browser that is capable of visualizing diverse types of genome-located data.",previewImage: urlJoin(NEXT_PUBLIC_BASE_PATH,"/images/jBrowse_Preview.png"),logoImage: urlJoin(NEXT_PUBLIC_BASE_PATH,"/images/jBrowse_Logo.png"),}}/><VizOptiononClick={selectViz(tableTypes.BAM_TABLE)}isEnabled={isIobioEnabled}viz={{title: "IOBIO",description: "Examine your sequence alignment file in seconds",previewImage: urlJoin(NEXT_PUBLIC_BASE_PATH,"/images/IOBIO_Preview.png"),logoImage: urlJoin(NEXT_PUBLIC_BASE_PATH,"/images/IOBIO_Logo.png"),}}/><VizOptiononClick={selectViz(tableTypes.CBIO_TABLE)}isEnabled={isCBioEnabled}viz={{title: "cBioPortal",description:
"Provides visualization, analysis and download of large-scale cancer genomics data sets.",previewImage: urlJoin(NEXT_PUBLIC_BASE_PATH,"/images/cBioPortal_Preview.png"),logoImage: urlJoin(NEXT_PUBLIC_BASE_PATH,"/images/cBioPortal_Logo.png"),}}/></VizModal>);}; |
demariadaniel
commented
Jun 23, 2025
|
demariadaniel
commented
Jun 24, 2025
OK this was a necessary & major improvement. I've made the changes as recommended, w/ CSS & functioning: Break Modal into components Could potentially break |
There was a problem hiding this comment.
18 TS errors from these changes
let's name consistently viz vs visualization
general pattern is to colocate styles and types with their components. I appreciate the cleanliness of your approach, unfortunately it's not the established pattern in this codebase.
ReactModal.setAppElement('#pageContent')is still in that "firstRender" useEffect dependency as well as in the ModalContainer. I think it should be scoped to the modal content. or at the very least be used in aonMounthook with an empty dependency array. If it does actually depend on "firstRender" please add a comment, very much sounds like it should work with a useEffect onMount scoped to the modal content somewhere. Not sure if using in both places intentionally.
| export const Badges = ({ isEnabled, badges }: { isEnabled: boolean; badges: BadgeItem[] }) => { | ||
| const theme = useTheme(); | ||
| const badgeGroup = badges.map((badge, index) => { | ||
| const badgeCss = badge.isAccent | ||
| ? accentBadgeStyle({ theme, isDisabled: !isEnabled }) | ||
| : badgeStyle({ theme, isDisabled: !isEnabled }); | ||
| return ( | ||
| <div key={`badge-${index}`} css={badgeCss}> | ||
| {badge.label} | ||
| </div> | ||
| ); | ||
| }); | ||
| return <>{badgeGroup}</>; | ||
| }; | ||
There was a problem hiding this comment.
I think these badges were bottom aligned before?
There was a problem hiding this comment.
Thanks, missing position: absolute, added
| import { badgeStyle, accentBadgeStyle, optionStyle } from './styles'; | ||
| import { BadgeItem, VizDetailProps } from './types'; | ||
| export const VizDetail = ({ title, description, previewImage, logoImage }: VizDetailProps) => { |
There was a problem hiding this comment.
"Viz" versus "Visualization"
I'd stick with one or the other, and I think we lean towards the full word.
(acknowledging that I used Viz in my suggestions, lots of quick stuff there that wasn't meant to be indicative of good naming, just better React)
There was a problem hiding this comment.
Naming, colocated styles, badge alignment, setAppElement are all addressed now, but I can't find the TS errors you're reporting? @ciaranschutte
There was a problem hiding this comment.
I did notice this content: url() is missing quotes which is messing up intellisense on my end https://github.com/overture-stack/stage/blob/main/components/pages/explorer/QueryBar.tsx#L74
But otherwise I'm able to run a successful build, no Type errors found
There was a problem hiding this comment.
looks ok now! Not sure where they came from
Summary
Adds Visualizer Modal to select between JBrowse, Iobio & cBio tables
Issues
Description of Changes
Creates VisualizerModal with updated Table Header navigation
Adds base logic for enabling/disabling different visualizer tables
Adds environment variables to feature flag JBrowse, Iobio & CBio
Special Instructions
Env requires new variable for Iobio testing:
NEXT_PUBLIC_IOBIO_ENABLED=trueEnv variables are also added for
NEXT_PUBLIC_JBROWSE_ENABLED&NEXT_PUBLIC_CBIOPORTAL_ENABLEDbut can be left disabledReadiness Checklist
.env.schemafile and documented in the README