-
Notifications
You must be signed in to change notification settings - Fork 31
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
Metadata uploader batch endpoints #1408
Metadata uploader batch endpoints #1408
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.
One thing to start. It's a little bigger so it's a part of it's own review. If you have any questions let me know!
src/main/java/ca/corefacility/bioinformatics/irida/ria/web/services/UIProjectSampleService.java
Outdated
Show resolved
Hide resolved
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 few more small things
src/main/webapp/resources/js/pages/projects/samples-metadata-import/redux/importReducer.ts
Outdated
Show resolved
Hide resolved
...a/ca/corefacility/bioinformatics/irida/ria/unit/web/services/UIProjectSampleServiceTest.java
Show resolved
Hide resolved
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 few small changes to the urls
...ca/corefacility/bioinformatics/irida/ria/web/ajax/projects/ProjectSamplesAjaxController.java
Outdated
Show resolved
Hide resolved
...ca/corefacility/bioinformatics/irida/ria/web/ajax/projects/ProjectSamplesAjaxController.java
Outdated
Show resolved
Hide resolved
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.
Overall this is starting to look good, just some comments on the chunk handling.
src/main/webapp/resources/js/pages/projects/samples-metadata-import/redux/importReducer.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/projects/samples-metadata-import/redux/importReducer.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/projects/samples-metadata-import/redux/importReducer.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/projects/samples-metadata-import/redux/importReducer.ts
Outdated
Show resolved
Hide resolved
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.
So I have been slowly going through your code today and I think I found a slightly major performance issue with the UI revolving around you component SampleMetadataImportWizard. The problem with it is that the way you structured it, every time you load in a different component (e.g. SampleMetadataImportReview) the entire UI needs to be redrawn, which really decreases the performance of the page. This can definitely be cleane up after this PR and should really result in any huge code changes, we can talk about it after this PR. These are a few of the things I do need you to look at. Also, please look anywhere that you have used any
any a TypeScript type, try to see if you can possibly type it, and if it cannot be, look into unknown
and see if that addresses it. I think in you array-utilities
this might work since you are passing in arbitraty arrays.
...ources/js/pages/projects/samples-metadata-import/components/SampleMetadataImportComplete.tsx
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/projects/samples-metadata-import/redux/import-utilities.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/resources/js/pages/projects/samples-metadata-import/redux/importReducer.ts
Show resolved
Hide resolved
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.
Thanks for all those changes
Description of changes
Related issue
#1400
Checklist
Things for the developer to confirm they've done before the PR should be accepted:
[ ] CHANGELOG.md (and UPGRADING.md if necessary) updated with information for new change.