Skip to content
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

89 extract contact us #110

Merged
merged 34 commits into from Nov 2, 2018
Merged

89 extract contact us #110

merged 34 commits into from Nov 2, 2018

Conversation

rorJeremy
Copy link
Contributor

WIP: Need to pull in the related specs for this work, but otherwise the code is mostly ready for review.

closes #89

Decided to rename this from 'contact_us' to 'contact_form' due to conflicts
when dealing with the singular vs plural forms of 'contact_us'. Also, right
now it seems as though I can't get an app to use the rails route helper methods
like _url or _path unless I call those helper methods combined with these other
helper methods you get whenever you mount an engine, so you end up having to
do 'wcc_contentful_app.contact_form_path' and then in the actuall application, you
have to start doing things like 'main_app.root_path' wherever the engine routes
and main app routes are both used. I don't know if this is just the way it has to
be, but I'm thinking there's probably a better way, I just haven't seen it yet.
I included formField with section_contact_form because you can't really use
the section_contact_form right now without having form_fields.
the contact-form.js will handle the contact form submissions, but for this js file to be
loaded in the main application, you need to include it like this:
<%= javascript_include_tag "wcc/contentful/app/application.js" %>
To generate this migration from the engine into your main application run the
following command from the main app:
rails wcc_contentful_app:install:migrations
@coveralls
Copy link

coveralls commented Oct 29, 2018

Coverage Status

Coverage decreased (-0.3%) to 94.894% when pulling c41338a on 89_extract_contact_us into 6d0882b on master.


<div class="contact-panel__content">
<!-- TODO: use route helper without needing to call directly on wcc_contentful_app -->
<%= form_tag wcc_contentful_app.contact_form_path, remote: true, data: { contact_form: 'yaddiyaddi' }, method: 'post' do %>
Copy link
Member

Choose a reason for hiding this comment

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

what is 'yaddiyaddi'?

Copy link
Member

Choose a reason for hiding this comment

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

# frozen_string_literal: true

module WCC::Contentful::App
class ContactFormSubmission < ApplicationRecord
Copy link
Member

Choose a reason for hiding this comment

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

does this cause an error if the table doesn't exist?

@@ -2,4 +2,5 @@

WCC::Contentful::App::Engine.routes.draw do
get '/:slug', to: 'pages#show'
post '/contact_form', to: 'contact_form#create'
Copy link
Member

Choose a reason for hiding this comment

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

as: :contact_form should provide you a route helper per the comment above

width: 1px;
}

#svg--wm--resources {
Copy link
Member

Choose a reason for hiding this comment

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

This is a watermarkresources thing. Do we even need the layout for this email? It can be a plain text email right?

Copy link
Member

@jpowell jpowell left a comment

Choose a reason for hiding this comment

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

One more goback - sorry that the issue didn't dictate this but a previous (undocumented) conversation... and the code smell

.table_exists? 'wcc_contentful_app_contact_form_submissions'

::WCC::Contentful::App::ContactFormSubmission.create!(
full_name: data['First and Last Name'],
Copy link
Member

Choose a reason for hiding this comment

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

I think the fact that these are hardcoded leans towards changing the implementation of the contact form fields we use here. Sorry - this is late, but Gordon and I were talking about this and remembered a conversation @frizman21 said we should go with the more simple (less configurable) approach.

See https://github.com/watermarkchurch/theporch-live/blob/develop/db/migrate/20180905202327_create_simple_mailchimp_form.ts and other relevant files... shouldn't be too much work to modify

Choose a reason for hiding this comment

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

Let's have a conversation before we re-open this...

Choose a reason for hiding this comment

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

@jpowell Per our conversation, please knock it out and let's move on past this task!
(including this note to close the thread)

@jpowell jpowell force-pushed the 89_extract_contact_us branch 5 times, most recently from 48133ee to 3297bb5 Compare November 2, 2018 18:54
@watermarkchurch-bot
Copy link

1 Warning
⚠️ Please limit commit subject line to 72 characters.
65a826d

Generated by 🚫 Danger

@jpowell jpowell merged commit 37f3d02 into master Nov 2, 2018
@jpowell jpowell deleted the 89_extract_contact_us branch November 2, 2018 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contact Us views and controller
6 participants