-
Notifications
You must be signed in to change notification settings - Fork 2
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
Austenem/CAT-922 Remove QA workspace option #3572
Conversation
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.
Unfortunately, I ran into a runtime error on non-dataset pages due to the changes to visualization props, but cutting down on prop drilling is definitely a good idea!
const { mapped_data_access_level } = useDetailContext(); | ||
const { | ||
datasetDetails: { hubmap_id }, | ||
} = useProcessedDatasetDetails(uuid); |
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.
These hooks don't work on preview
and organ
pages:
One potential option, similar to your feedback on my PR, would be to pull the logic for return null
here up into the VisualizationWrapper
and pass displayVisualizationNotebook
as a prop - that would at least let us decrease the amount of prop drilling that's done solely for this functionality.
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.
Ah yikes! Good catch. I think that's a good idea - it looks likehasNotebook
is already representing that (it's only true for the dataset detail pages), so I'll check that before trying to access the detailContext
.
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.
Nicely done! Not sure how I overlooked hasNotebook
😆
Summary
Fixes issue of Workspaces button being available for protected datasets in the Visualization section.
Design Documentation/Original Tickets
CAT-922 Jira ticket
Testing
Tested protected and public datasets to ensure Workspaces button appeared only for visualizations of public + published datasets.
Screenshots/Video
Local on left, prod on right:
Checklist
CHANGELOG-your-feature-name-here.md
is present in the root directory, describing the change(s) in full sentences.