-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add a function to get updated spec (onAltGoslingSpecUpdate
)
#52
Conversation
const examples = {'Bar chart': barChart, | ||
'Heatmap': heatmap, | ||
'Matrix': matrix, | ||
'Comparison of four samples': doubleMarks, | ||
'Annotated chart': ruleMark, | ||
'Comparing two samples': compareTwoSamples, | ||
'Linked views': brush, | ||
'Circular halves': circularHalves, | ||
'Gene annotations': geneAnnotation, | ||
'Ideogram expression': ideogramWithArea, | ||
}; | ||
const examples = { | ||
'Bar chart': barChart, | ||
'Heatmap': heatmap, | ||
'Matrix': matrix, | ||
'Comparison of four samples': doubleMarks, | ||
'Annotated chart': ruleMark, | ||
'Comparing two samples': compareTwoSamples, | ||
'Linked views': brush, | ||
'Circular halves': circularHalves, | ||
'Gene annotations': geneAnnotation, | ||
'Ideogram expression': ideogramWithArea, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably had to turn off my auto-linting. All formatting changes reflect the .eslintrc.cjs
option defined in this project.
{ selectedExample === 'editor' ? | ||
(editorValid === 'invalid' ? | ||
<Typography variant='body1' color='error'>No Gosling or AltGosling components could be loaded.</Typography> | ||
: <AltGoslingComponent spec={editorText} download name={selectedExample} /> | ||
) | ||
: <AltGoslingComponent spec={examples[selectedExample]} download name={selectedExample} /> | ||
} | ||
{(() => { | ||
let goslingSpec: string; | ||
if (selectedExample === 'editor' && editorValid === 'invalid') { | ||
return <Typography variant='body1' color='error'>No Gosling or AltGosling components could be loaded.</Typography>; | ||
} else if (selectedExample === 'editor' && editorValid !== 'invalid') { | ||
goslingSpec = editorText; | ||
} else { | ||
goslingSpec = examples[selectedExample]; | ||
} | ||
return <AltGoslingComponent spec={goslingSpec} download name={selectedExample} onAltGoslingSpecUpdate={spec => { | ||
console.log('AltGosling Spec Updated', spec); | ||
}} />; | ||
})()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only part I edited to include onAltGoslingSpecUpdate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the structure a bit to make it more readable and to avoid duplicated <AltGoslingComponent/>
use
// Update current alt | ||
updateAltPanelDisplay(altSpec); | ||
// Reset data panels | ||
setDataPanelCurrent(undefined); | ||
setDataPanelPrevious(undefined); | ||
// setExpandedAltPanelWrapper(['tree']); | ||
// setExpandedDataPanelWrapper(['tree']); | ||
|
||
// Callback function to pass over the AltGoslingSpec | ||
props.onAltGoslingSpecUpdate?.(altSpec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A place to call the onAltGoslingSpecUpdate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add this to the AltGoslingCompProps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This would allow a user to change change behaviour as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I already added it as well
@@ -38,17 +38,18 @@ interface AltGoslingCompProps extends GoslingCompProps { | |||
layoutPanels?: 'vertical' | 'horizontal'; | |||
download?: boolean; | |||
dataTableRoundValues?: boolean; | |||
onAltGoslingSpecUpdate?: (altSpec: AltGoslingSpec) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see now! I had missed this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
This PR brings only the change needed to support
onAltGoslingSpecUpdate
from #48.Usage: