Skip to content

Contact - Pipedrive - #31

Open
sachinmjadhav wants to merge 7 commits into
masterfrom
contact-pipedrive
Open

Contact - Pipedrive#31
sachinmjadhav wants to merge 7 commits into
masterfrom
contact-pipedrive

Conversation

@sachinmjadhav

@sachinmjadhavsachinmjadhav commented Jul 23, 2019

Copy link
Copy Markdown
Contributor

Integrated pipedrive with contact form

Kameshwaran
Kameshwaran requested changes Jul 24, 2019
View reviewed changes
Comment threadsrc/pages/thank-you.js Outdated
Comment threadsrc/pages/thank-you.js Outdated
Comment threadsrc/templates/header.js
Comment threadsrc/templates/header.js Outdated
Comment threadsrc/templates/header.js Outdated
Comment threadsrc/templates/header.js Outdated
Comment threadsrc/templates/header.js Outdated
Comment threadsrc/templates/header.js
Kameshwaran
Kameshwaran reviewed Jul 24, 2019
View reviewed changes
Comment threadsrc/pages/contact.mdx
Kameshwaran
Kameshwaran requested changes Jul 24, 2019
View reviewed changes
Comment threadconfig.js Outdated
Comment threadsrc/pages/thank-you.js Outdated
Comment threadsrc/pages/contact.mdx
Comment threadsrc/pages/thank-you.js Outdated
Comment threadsrc/pages/thank-you.js Outdated
Comment threadsrc/templates/header.js
@Kameshwaran

Kameshwaran commented Jul 25, 2019

Copy link
Copy Markdown
Contributor

PR looks ok to me.

Kameshwaran
Kameshwaran reviewed Jul 25, 2019
View reviewed changes
Comment threadsrc/templates/header.js
sanandnarayan
sanandnarayan requested changes Jul 26, 2019
View reviewed changes

@sanandnarayansanandnarayan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have some way to have the js run only in the contact page. this is not urgent so we can do this correctly

Comment threadsrc/templates/header.js
if(typeof window !== 'undefined') {
let userDetails = {}
let SITE_NAME = window.location.origin
document.getElementById('contact-form') &&

@sanandnarayansanandnarayanJul 26, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kameshwaran should we put this in JS only in the contact page. in header.js means it will come in every page right?

@KameshwaranKameshwaranJul 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it in the header.
This JS not only contains form handling logic, but also tracks the list of pages browsed by the visitor. That info also sent in the form when a visitor submits it.

@KameshwaranKameshwaranJul 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanandnarayan

@KameshwaranKameshwaranAug 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanandnarayan

@sanandnarayan

sanandnarayan commented Jul 26, 2019

Copy link
Copy Markdown
Member

may be the form logic should be a part of the form component , and not a part of the header template?

@sanandnarayan
sanandnarayan self-requested a review July 26, 2019 15:31
@sanandnarayan
sanandnarayan requested review from Kameshwaran and removed request for sanandnarayanJuly 26, 2019 15:31
@Kameshwaran

Kameshwaran commented Aug 5, 2019

Copy link
Copy Markdown
Contributor

may be the form logic should be a part of the form component , and not a part of the header template?

Replied here.

sanandnarayan
sanandnarayan reviewed Aug 5, 2019
View reviewed changes
Comment threadsrc/pages/thank-you.js Outdated

const ThankYou = () => {
return (
<CenteredDiv>

@sanandnarayansanandnarayanAug 5, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://theme-ui.com/guides/layouts , can we use main component from https://theme-ui.com/guides/layouts instead of centered div

Comment threadsrc/templates/header.js Outdated
const idea = document.getElementById('idea').value
const email = document.getElementById('email').value
document.getElementById('contact-submit').disabled = true
document.getElementById('idea').value = ""

@sanandnarayansanandnarayanAug 5, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kameshwaran why reset values even when there is an error?

@KameshwaranKameshwaranAug 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thanks.

@Kameshwaran

Kameshwaran commented Aug 5, 2019

Copy link
Copy Markdown
Contributor

@sachinmjadhav please make the fixes commented by @sanandnarayan

@sanandnarayan

sanandnarayan commented Aug 6, 2019

Copy link
Copy Markdown
Member

@sachinmjadhav you can deprioritize this for now. i am kinda liking formspree with its spam filters. also we added chat to our website, so let this PR be open for now @Kameshwaran

@Kameshwaran

Kameshwaran commented Aug 6, 2019

Copy link
Copy Markdown
Contributor

This PR is important for us.
The current website does not have '/thank-you' page after submitting the form.
The '/thank-you' page is very important for us, as goal conversion is tracked by google analytics only on that page. @sanandnarayan

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sanandnarayansanandnarayansanandnarayan requested changes
@KameshwaranKameshwaranAwaiting requested review from Kameshwaran

Assignees

@sanandnarayansanandnarayan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sachinmjadhav@Kameshwaran@sanandnarayan