-
Notifications
You must be signed in to change notification settings - Fork 4
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
DCJ-367: Link to data library instead of old catalog #2615
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.
Looks good as far as I can tell -- I agree with your choice to limit the scope to updating links only. Will you file that follow-on ticket for full removal of the DatasetCatalog
component and related code?
@@ -82,7 +82,7 @@ export const headerTabsConfig = [ | |||
link: '/admin_manage_dar_collections', | |||
children: [ | |||
{ label: 'DAR Requests', link: '/admin_manage_dar_collections' }, | |||
{ label: 'Dataset Catalog', link: '/dataset_catalog' }, | |||
{ label: 'Data Library', link: '/datalibrary', search: 'datalibrary' }, |
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.
What does this search
field represent?
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.
The search
value is used in a couple places in this file. I don't remember exactly how it works, but I'm pretty sure it has to do with displaying the correct tab based on your role and where you have navigated to/from. For example, if you have multiple roles, each one with access to the same sub-tab (i.e. data library), then this feature helps restore the correct sub-tab when the user navigates between higher level role tabs.
@@ -98,7 +97,6 @@ const Routes = (props) => ( | |||
<AuthenticatedRoute path="/admin_manage_lc/" component={AdminManageLC} props={props} rolesAllowed={[USER_ROLES.admin]} /> | |||
<AuthenticatedRoute path="/admin_manage_dar_collections/" component={AdminManageDarCollections} props={props} rolesAllowed={[USER_ROLES.admin]} /> | |||
{checkEnv(envGroups.NON_STAGING) && <AuthenticatedRoute path="/dataset_catalog/:variant" component={CustomDatasetCatalog} props={props} rolesAllowed={[USER_ROLES.researcher]}/>} |
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.
Is this a place that also needs to be updated?
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.
This is the custom dataset catalog functionality
in the PR description that I did not want to remove yet. Jonathan has shared out links to this so we need to figure out what to do with it.
Yes, thank you, I will get that ticket into the backlog. |
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.
looks good to me! 👍
Addresses
https://broadworkbench.atlassian.net/browse/DCJ-367
Summary
Replace all links to the old catalog page to the new data library page
This PR does not remove the older custom dataset catalog functionality. There are no in-app links to that feature but it has been shared with customers. This should be addressed in a separate ticket as a redirect feature.
Have you read Terra's Contributing Guide lately? If not, do that first.