Skip to content
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

Add warning banner for download table preview for entities with lots of vars #1673

Merged
merged 9 commits into from
Mar 7, 2023

Conversation

asizemore
Copy link
Member

Resolves #1652

Current behavior
Screen Shot 2023-03-03 at 10 58 13 AM

Now it looks like:
Screen Shot 2023-03-03 at 10 58 38 AM

That error banner only comes up once i've clicked at least one var.

Is it weird to have both banners? I thought about removing the server-error banner, but i dont want to hide errors that might be useful to us to see later...

@dmfalke
Copy link
Member

dmfalke commented Mar 3, 2023

I think we want to avoid making the request if showPreviewTableWarning === true.

I would rename that variable to something like canLoadTablePreview and define it closer to the top of the component, and use that value as a part of this condition.

@dmfalke
Copy link
Member

dmfalke commented Mar 3, 2023

I would rename that variable to something like canLoadTablePreview and define it closer to the top of the component, and use that value as a part of this condition.

Hmm... maybe this would be a better place to add the check.

@asizemore
Copy link
Member Author

Thanks @dmfalke !
Now after the update it looks like:

Screen Shot 2023-03-06 at 2 01 55 PM

@asizemore asizemore marked this pull request as ready for review March 6, 2023 19:02
@asizemore asizemore requested a review from dmfalke March 6, 2023 19:02
Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once my comment is addressed, I think this is ready to go!

@@ -223,7 +229,7 @@ export default function SubsetDownloadModal({
>['serverSidePagination']['fetchPaginatedData'] = useCallback(
({ pageSize, pageIndex, sortBy }) => {
if (!currentEntity) return;
if (selectedVariableDescriptors.length === 0) {
if (selectedVariableDescriptors.length === 0 || !canLoadTablePreview) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canLoadTablePreview should probably be added to the list of dependencies for this callback function (

[
selectedVariableDescriptors,
studyMetadata.id,
subsettingClient,
analysisState.analysis?.descriptor.subset.descriptor,
currentEntity,
mergeKeys,
]
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, because if this value changed we'd definitely want to stop/start fetching data. Thank you!

@asizemore asizemore merged commit 3f5373b into main Mar 7, 2023
@asizemore asizemore deleted the fix/1652/show-helpful-message-mbio-assay-downloads branch March 7, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: show user-friendly message when subset results table preview cannot be generated
2 participants