[BIOMAGE-1273] Refactor ProjectDetails component #458
Conversation
Codecov Report
@@ Coverage Diff @@
## master #458 +/- ##
==========================================
+ Coverage 60.56% 61.54% +0.98%
==========================================
Files 137 142 +5
Lines 4488 4525 +37
Branches 880 875 -5
==========================================
+ Hits 2718 2785 +67
+ Misses 1748 1719 -29
+ Partials 22 21 -1
Continue to review full report at Codecov.
|
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable import/no-unresolved */ | |||
There was a problem hiding this comment.
because we can import stuff without a relative path but like 'utils/something.js' eslint still marks it as unresolved though.
There was a problem hiding this comment.
This is because we added baseUrl in jsconfig.json : https://nextjs.org/docs/advanced-features/module-path-aliases, but it's not yet widely used it seems
There was a problem hiding this comment.
what is it about? If it is not used, should we remove it?
There was a problem hiding this comment.
I'm using it in some places i find it convenient to import things without gettting lost in the path
There was a problem hiding this comment.
I didn't know that this had been added, is there a ticket for making the existing relative paths absolute?
Also we should disable this eslint policy in the whole project if it is necessary to add it in every single file we use this.
| disabled={ | ||
| projects.ids.length === 0 | ||
| || activeProject?.samples?.length === 0 | ||
| || isAddingMetadata |
There was a problem hiding this comment.
this isAddingMetadata doesn't read very well, or at least it is not obvious to me what it is about. Can you explain?
There was a problem hiding this comment.
We use it when the user is in the middle of adding metadata - disable the Add metadata button so they can't click it again while creating metadata
| import pipelineStatus from '../../utils/pipelineStatusValues'; | ||
| import downloadData from '../../utils/data-management/downloadExperimentData'; | ||
| import downloadTypes from '../../utils/data-management/downloadTypes'; | ||
| import { exportQCParameters, filterQCParameters } from '../../utils/exportQCParameters'; |
There was a problem hiding this comment.
I would move this exportQCParameters file to the data management folder in utils
src/__test__/components/data-management/ProjectDetails.test.jsx
Outdated
Show resolved
Hide resolved
src/__test__/components/data-management/ProjectDetails.test.jsx
Outdated
Show resolved
Hide resolved
|
|
||
| if (status === UploadStatus.UPLOADED) { | ||
| return ( | ||
| <div |
There was a problem hiding this comment.
I feel like we could perhaps separate each of these into other files, one for each type of table cell (for each status)?
There was a problem hiding this comment.
It would make this file significantly shorter
There was a problem hiding this comment.
Wouldn't this make it a bigger mess - like 4 other components which are used in only one place? We already have a lot of stuff in components/data-management, plus managing the passing of props and etc. that file is not that big in my opinion, what do you think?
There was a problem hiding this comment.
I was thinking that moving these 4 new files into a new folder would deal with that problem but it is not that important, you so leaving as it is is fine too.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable import/no-unresolved */ | |||
There was a problem hiding this comment.
I didn't know that this had been added, is there a ticket for making the existing relative paths absolute?
Also we should disable this eslint policy in the whole project if it is necessary to add it in every single file we use this.
|
"I didn't know that this had been added, is there a ticket for making the existing relative paths absolute?" No there isn't. I don't think we should change every path to absolute - we already have them defined correctly so what's the point? I was thinking to use this in further imports you make so its easier |
…td/ui into rework-project-details
| import { exportQCParameters, filterQCParameters } from '../../utils/data-management/exportQCParameters'; | ||
| import { loadBackendStatus } from '../../redux/actions/backendStatus/index'; | ||
|
|
||
| const DownloadData = (props) => { |
There was a problem hiding this comment.
I don't see any unit tests for this component
There was a problem hiding this comment.
we can add them as part of your next PR
…td/ui into rework-project-details
Whenever we move a file we need to adapt all of the relative imports defined in it, which means extra work whenever we reorganize directories |
StefanBabukov
dismissed
cosa65’s stale review
already approved by Marcell today
Background
Link to issue
https://biomage.atlassian.net/secure/RapidBoard.jspa?rapidView=1&projectKey=BIOMAGE&modal=detail&selectedIssue=BIOMAGE-1273
Link to staging deployment URL
https://ui-stefan-ui458.scp-staging.biomage.net/
Links to any Pull Requests related to this
Anything else the reviewers should know about the changes here
Changes
Code changes
Definition of DONE
Your changes will be ready for merging after each of the steps below have been completed:
Testing
To set up easy local testing with inframock, follow the instructions here: https://github.com/biomage-ltd/inframock
To deploy to the staging environment, follow the instructions here: https://github.com/biomage-ltd/biomage-utils
Documentation updates
Is all relevant documentation updated to reflect the proposed changes in this PR?
Approvers
Just before merging:
unstagescript in here: https://github.com/biomage-ltd/biomage-utils is executed. This script cleans up your deployment to stagingOptional