-
Notifications
You must be signed in to change notification settings - Fork 18
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
Contact Collection and Dataset owner feature #609
base: develop
Are you sure you want to change the base?
Contact Collection and Dataset owner feature #609
Conversation
1e33bdb
to
5cedc6c
Compare
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 left some suggestions to improve the code and organize it in a better way.
I think the way you name the contact repository methods is a bit confusing, ContactDTO
, submitContactInfo
, useSubmitContact
. Maybe something more related to what you are doing which is sending feedback to the owners.
On the other hand I think you had a confusion between repositories and factories, factories is a convention that the project has to create or instantiate the components at page level.
And repositories must be created only once in these factories and passed through props to the component that wants to use it.
Also you can simplify success message with a toast instead of an alert.
Please take a look at all the comments first and then see what needs to be changed.
If you need any help let me know, after these changes I will analyze the UI and functionality in more depth.
src/sections/dataset/dataset-action-buttons/DatasetActionButtons.tsx
Outdated
Show resolved
Hide resolved
@g-saracca Hi German, thanks for your feedback. I made changes regarding to reviews. Please help to check them again, ty! |
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.
Some more comments, thanks for addressing previous requested changes.
src/sections/shared/form/ContactForm/useSendFeedbacktoOwners.tsx
Outdated
Show resolved
Hide resolved
tests/component/sections/dataset/dataset-action-buttons/DatasetActionButtons.spec.tsx
Outdated
Show resolved
Hide resolved
tests/component/sections/shared/pagination/contact-modal/ContactModal.spec.tsx
Outdated
Show resolved
Hide resolved
Changes applied. Thanks for taking care of this 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.
Looks great! Approving, nice work!
I've approved but I saw that component test coverage is not reaching the branch coverage min of 95%. |
I think you are missing testing closing the dialog? See https://coveralls.io/builds/72453589/source?filename=src%2Fsections%2Fshared%2Fcontact%2Fcontact-modal%2Fcontact-modal.tsx#L74 |
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! Approving!
What this PR does / why we need it:
The goal is to replicate the Contact feature on the Collection Page and Dataset Page.
Which issue(s) this PR closes:
Special notes for your reviewer:
Not sure contact factory is written in a correct way or not to create contact repository
Suggestions on how to test this:
Show a success alert on the page after submission.
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: