-
Notifications
You must be signed in to change notification settings - Fork 3
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
Expand info on matching dataset cards Fixes #377 #421
Expand info on matching dataset cards Fixes #377 #421
Conversation
…nd aggregate dataset indicators
Reviewer's Guide by SourceryThis pull request enhances the ResultCard component to display whether a dataset is a DataLad dataset or an aggregate dataset. This addresses issue #377. Class diagram showing updated ResultCard component propsclassDiagram
class ResultCard {
+String nodeName
+String datasetName
+String datasetUUID
+String datasetDescription
+Number datasetTotalSubjects
+Number numMatchingSubjects
+String[] imageModals
+Object pipelines
+Boolean checked
+Function onCheckboxChange
+Boolean isDataLad
+Boolean isAggregate
}
note for ResultCard "Added isDataLad and isAggregate props"
class ResultContainer {
-String[] download
-Boolean selectAll
+updateDownload()
+render()
}
ResultContainer --> ResultCard : passes props
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
❌ Deploy Preview for neurobagel-query failed.
|
Hey @ShubhamShekhawat, thanks for the PR and your willingness to contribute to Neurobagel. I think there is a bit of a misunderstanding between the (admittedly very sparse) issue and your PR. So I suggest we first have a bit of a chat on the issue about your plan so we can help you get more context. See also https://neurobagel.org/contributing/CONTRIBUTING/ first! In general, this is a tricky issue and there might be easier ones for you to get started on. Let us know if you'd like some pointers to those as well. For now, feel free to keep this PR open if you still plan on addressing the issue after more discussion or to close it if you you'd rather work on something else. |
Hey @ShubhamShekhawat, I'm going to close this PR for now. If you are interested in picking it back up, please ping us on the issue so we can discuss your implementation with you. |
Hi @ShubhamShekhawat, to expand on @surchs' points above: The focus of the linked issue, and the key thing that was missing from the current PR, is actual logic for determining whether the dataset is aggregated or not, or a DataLad dataset or not, based on the response the query tool receives from its corresponding API. Since this may not have been immediately obvious in the original issue, I've updated the issue description with more details. Like Seb said, feel free to have a look and let us know if you'd like to have another stab at it. Side note: we encourage you to check that our automated tests and checks are passing for your PR! In this case, the failing checks could highlight areas where additional tests may help to verify the behaviour of any new code. |
isDataLad
andisAggregate
indicators toResultCard
component.ResultContainer
toResultCard
.Fixes Expand info on matching dataset cards #377
@alyssadai Mam please check and if something is wrong let me know.
Summary by Sourcery
New Features: