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.

Sign up for GitHub

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

Jump to bottom

Download table data #848

Merged
merged 12 commits into from
Oct 14, 2022
Merged

Download table data #848

merged 12 commits into from
Oct 14, 2022

Conversation

msmanoj
Copy link
Contributor

@msmanoj msmanoj commented Oct 3, 2022 •
edited
Loading

Description

  • Implemented "Download this table" option on DataTable and enable it on Blast result table

The DataTable can either handle the download by itself by generating a csv or it can give the control over to the parent component using the downloadHandler prop.

Related JIRA Issue(s)

https://www.ebi.ac.uk/panda/jira/browse/ENSWBSITES-1736

Deployment URL(s)

http://datatable-export.review.ensembl.org

@msmanoj msmanoj requested a review from azangru October 4, 2022 13:55
@msmanoj msmanoj marked this pull request as ready for review October 4, 2022 15:08
@ens-st3
Copy link
Contributor

ens-st3 commented Oct 4, 2022

Download works as expected.
What do we expect to happen after I've clicked the 'Download' button ? Screen to stay as it is, ie still showing the 'Download', or reset and 'Actions' in the drop down
Screenshot 2022-10-04 at 16 59 01
Screenshot 2022-10-04 at 16 56 08

@imransl
Copy link
Contributor

imransl commented Oct 11, 2022

Reviewed the PR again and haven't got much to add

azangru
azangru reviewed Oct 13, 2022
View reviewed changes
...components/table-controls/components/table-actions/components/download-data/DownloadData.tsx Outdated
})
: cell;

dataForExport[rowIndex + 1].push(getReactNodeText(cellExportData));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't you want to check here if cellExportData is a primitive (string / number), in which case you could push it into dataForExport directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's done. Previously it was getting checked inside getReactNodeText but not since we updated it.

azangru
azangru reviewed Oct 13, 2022
View reviewed changes
...components/table-controls/components/table-actions/components/download-data/DownloadData.tsx
});
}
}, 1000);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, we messed up. There is now a second's delay when you click on "Cancel", which shouldn't be there.

Screen.Recording.2022-10-13.at.08.29.39.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now fixed.

azangru
azangru reviewed Oct 13, 2022
View reviewed changes
...components/table-controls/components/table-actions/components/download-data/DownloadData.tsx Outdated
return (
<div className={styles.downloadData}>
<span>{downloadFileName ?? 'table.csv'}</span>
<LoadingButton onClick={handleDownload}>Download</LoadingButton>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is super minor, but since all data is already on the client, the download completes before the spinner stops:

Screen.Recording.2022-10-13.at.08.47.54.mov

It can be addressed by changing from LoadingButton to ControlledLoadingButton, and driving the state of that button from the DownloadData component. Or we can ignore this for now, and see how #841 pans out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have addressed this issue in the latest commit.

@azangru
Copy link
Collaborator

azangru commented Oct 13, 2022

The downloaded table isn't default-sorted (by e-value). Is this correct? @ens-st3 ?

@azangru
Copy link
Collaborator

azangru commented Oct 13, 2022

After sorting the downloaded data locally, I am seeing a discrepancy in the Score column between what's downloaded and what's shown in the table site. Must be an error somewhere — could easily be in my download code, or could be in the table.

image

image

@msmanoj
Copy link
Contributor Author

msmanoj commented Oct 13, 2022

After sorting the downloaded data locally, I am seeing a discrepancy in the Score column between what's downloaded and what's shown in the table site. Must be an error somewhere — could easily be in my download code, or could be in the table.

image

image

This is because I used hsp_score instead of hsp_bit_score. I wonder which one is correct?

azangru
azangru reviewed Oct 13, 2022
View reviewed changes
...components/table-controls/components/table-actions/components/download-data/DownloadData.tsx Outdated
setTimeout(restoreDefaults, 1000);
} catch {
setDownloadState(LoadingState.ERROR);
setTimeout(() => setDownloadState(LoadingState.NOT_REQUESTED), 2000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should probably also check for allowComponentResetRef.current in the callback to this setTimeout function. You are changing component's state here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's done

azangru
azangru reviewed Oct 13, 2022
View reviewed changes
...components/table-controls/components/table-actions/components/download-data/DownloadData.tsx
setTimeout(() => setDownloadState(LoadingState.NOT_REQUESTED), 2000);
}

return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not returning within the try block so this return here is required

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, sorry, didn't see where the function ends 🙂

@msmanoj msmanoj merged commit e6ade07 into dev Oct 14, 2022
@msmanoj msmanoj deleted the datatable-export branch October 14, 2022 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imransl imransl imransl left review comments

@azangru azangru azangru approved these changes

@ridwanamo ridwanamo Awaiting requested review from ridwanamo

@ens-st3 ens-st3 Awaiting requested review from ens-st3

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@msmanoj @ens-st3 @imransl @azangru