-
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
339 add alerts for create dataset #354
Conversation
@MellyGray I added some logic to the submitForm() function to save the "created" state before navigating to the View Dataset page, but I wasn't sure where to use the location hook on that page. I tried adding it to the DatasetFactory or the DatasetProvider, but the logic seemed a little awkward there. Do you have a idea for where logic like this should go? |
@ekraffmiller I would place the logic to read the state from the location hook in the DatasetFactory, simply because it's where we're reading the rest of the navigation parameters. I guess what you mean by 'awkward' is because usually, the Dataset Alerts come from the repository, so we need to pass the state all the way through the repository. That's the part that seems awkward to me. Maybe we can pass the 'created' state to the Dataset.tsx component as a new prop and then use the useAlertContext to add a new Dataset Alert from the Dataset.tsx component instead of from the repository. Alternatively, we could call the useAlertContext in the DatasetFactory component, but then we wouldn't be able to test it individually. I don't have a strong opinion on this |
Thanks @MellyGray, I decided to add a 'created' param to Dataset.tsx, to keep the repository logic simpler |
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.
LGTM! 🎉
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!
testalerts.mov
339 add alerts for create dataset
What this PR does / why we need it: Adds Alerts for Dataset Validation Errors, and Create Dataset Success.
Which issue(s) this PR closes:
Special notes for your reviewer:
Sometimes the e2e getTotalDatasetsCount() fails, because of this issue: #294
Suggestions on how to test this
Review Dataset 'Created' Story, Create a Dataset and check for Validation Alerts for missing required fields, and check for Success message on Dataset Page after dataset is created.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
see mockups in #339
Is there a release notes update needed for this change?:
Additional documentation: