-
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
Implement contact integration to collection and dataset page #609
base: develop
Are you sure you want to change the base?
Implement contact integration to collection and dataset page #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/contact/domain/models/Contact.ts
Outdated
@@ -0,0 +1,5 @@ | |||
export interface Contact { |
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 it ok to name it contact? isn't this the info that the use case return after success?
But isn't the same info you are sending?
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.
It has a little bit difference, the reply message's body would be Hello ${User Name},\n\nYou have just been sent the following message from ${FromEmail} via the Root hosted dataset ...
but I didn't use the reply message in Contact part in front end repo. I'll rename it somehow meaningful
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: