-
Notifications
You must be signed in to change notification settings - Fork 44
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
new changes for B3 WALDER project to make changes for discovery porta… #1551
base: master
Are you sure you want to change the base?
Conversation
Created temp branch for testing: |
src/Discovery/DiscoveryActionBar/components/OpenFillRequestFormButton.tsx
Outdated
Show resolved
Hide resolved
<React.Fragment> | ||
| ||
<a target='_blank' rel='noreferrer' href='https://pandemicresponsecommons.org'> | ||
{'Pandemic Response Commons Website'} |
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.
I hope we don't have to hardcode these env specific values in here
the url can be inferred from localconf, the text might be also about to be pulled from some value there
if not, maybe put them in configs
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.
I was planning to put it in config but I wasnt sure that it would be a good idea to just have config for value which will not change ever. I can add it into config if needed
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.
Put variable in the portal config for URL and Text, add external link icon
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.
external link icon in portal https://github.com/uc-cdis/data-portal/blob/master/src/components/layout/FooterNIAID.jsx#L162-L164
src/Discovery/DiscoveryActionBar/components/OpenFillRequestFormButton.tsx
Outdated
Show resolved
Hide resolved
} | ||
combinedIds += item._medical_sample_id; | ||
}); | ||
const url = `${props.config.features.exportToWorkspace.fillRequestFormURL}?query=${encodeURIComponent(combinedIds)}`; |
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.
const url = `${props.config.features.exportToWorkspace.fillRequestFormURL}?query=${encodeURIComponent(combinedIds)}`; | |
const url = `${props.config.features.exportToWorkspace.fillRequestFormURL}query=${encodeURIComponent(combinedIds)}`; |
No need to check here, already done in L28 by the ternary operator
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.
also combinedIds
here might be empty, unless _medical_sample_id
is guaranteed to exists in the metadata of all selectable studies, consider if you need some extra error handling for that case here
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.
also
combinedIds
here might be empty, unless_medical_sample_id
is guaranteed to exists in the metadata of all selectable studies, consider if you need some extra error handling for that case here
OCC is pushing data into this mds by a semi-automated system that parses the file, appends a medical ID, and then pushes it to mds. Even though we want to put an error handling, I have to put a UI error handling in this case, can you tell me what reference in gen3 I can take and replicate here?
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.
No need to check here, already done in L28 by the ternary operator
That is not a ternary operator, that is a URL generated with a query parameter that requires a question mark
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.
Disable checkbox if _medical_sample_id
doesn't exist also add check into code if combinedIds
are empty
const url = `${props.config.features.exportToWorkspace.fillRequestFormURL}?query=${encodeURIComponent(combinedIds)}`; | ||
window.open(url, '_blank'); | ||
} : () => { | ||
window.open(props.config.features.exportToWorkspace.fillRequestFormURL, '_blank'); |
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.
why it should open this URL in this case, where fillRequestFormURL
doesn't exists?
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.
Technically it should not redirect anywhere else as there is nothing setup
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.
if fillRequestFormURL
doesn't exist, hide the button request Access
src/Discovery/DiscoveryActionBar/components/OpenFillRequestFormButton.tsx
Outdated
Show resolved
Hide resolved
src/Discovery/DiscoveryActionBar/components/OpenFillRequestFormButton.tsx
Outdated
Show resolved
Hide resolved
src/Discovery/DiscoveryListView.tsx
Outdated
props.config.features.exportToWorkspace | ||
&& props.config.features.exportToWorkspace.enabled | ||
(props.config.features.exportToWorkspace | ||
&& props.config.features.exportToWorkspace.enabled) || ((props.config.features.exportToWorkspace?.enableFillRequestForm |
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.
we probably should consider renaming the config section exportToWorkspace
to something else since it is more like an action button bar rather than just for exporting to ws
but this is more like a note to ourselves, not a item that in the scope of your PR
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.
Are you still planning to merge PR when all comments are resolved? We’d recommend that as we have a deadline to be met
…rvi-occ/data-portal into feat/b3_walder_discovery_prc_2
…into feat/b3_walder_discovery_prc_2 resolving conflict
Improvements
changes in the discovery portal for the new search portal in PRC QA. I created this PR to test it in QA. I had previous PRs in #1392 and #1394 but I had to make a new commit to incorporate changes for new DiscoeryActionBar folder and to resolve conflicts.
Currently, This PR should NOT be merged, I only need this PR to be here so I can use this in QA COVID 19
similar to this
OCC is working on getting this PR to be merged by end of Q3 2024