-
Notifications
You must be signed in to change notification settings - Fork 16
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
Feature/340 collection host input #399
Conversation
…orm' into feature/340-collection-host-input
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'm adding a suggestion but I'm so happy that the collection is read only until you click edit! 🎉
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.
@ekraffmiller Looks great, just a few comments.
@pdurbin thanks for that catch!
src/sections/create-dataset/HostCollectionForm/HostCollectionForm.tsx
Outdated
Show resolved
Hide resolved
src/sections/create-dataset/HostCollectionForm/HostCollectionForm.tsx
Outdated
Show resolved
Hide resolved
src/sections/create-dataset/HostCollectionForm/HostCollectionForm.tsx
Outdated
Show resolved
Hide resolved
@@ -42,15 +45,20 @@ export function CreateDataset({ | |||
|
|||
return ( | |||
<article> | |||
<NotImplementedModal show={isModalOpen} handleClose={hideModal} /> |
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.
Should live outside the article element, you can use a react fragment like this:
return (
<>
<article>
.....
</article>
<NotImplementedModal show={isModalOpen} handleClose={hideModal} />
<>
)
@GermanSaracca I made the changes you suggested, thank you |
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.
Looking good! Approved
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! Just tested it for a new dataset under root and under a subcollection and displays the correct collection alias.
Feature/340 collection host input
What this PR does / why we need it:
Adds a read only input field for the host collection in Create Dataset Page
Which issue(s) this PR closes:
Closes #340
Special notes for your reviewer:
Suggestions on how to test this:
Create a new collection then navigate to the collection page. From the collection page, select the "Add Dataset" dropdown. On the Create Dataset page, the Host Collection input should be prefilled with the alias of the new collection.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
no
Additional documentation:
none