Skip to content

STENCIL-2934 Add recaptcha v2 support to cornerstone.#951

Merged
mcampa merged 1 commit into
bigcommerce:masterfrom
junedkazi:STENCIL-2932-recaptcha-v2
Mar 10, 2017
Merged

STENCIL-2934 Add recaptcha v2 support to cornerstone.#951
mcampa merged 1 commit into
bigcommerce:masterfrom
junedkazi:STENCIL-2932-recaptcha-v2

Conversation

@junedkazi

@junedkazi junedkazi commented Mar 7, 2017

Copy link
Copy Markdown
Contributor

What?

Adding support for google recaptcha v2.

Tickets / Documentation

Add links to any relevant tickets and documentation.

Do Not Merge

It depends on https://github.com/bigcommerce/bigcommerce/pull/17715.

zip:
Cornerstone-recaptcha-v2-1.5.3.zip

Comment thread lang/en.json
"new": "Write a Review",
"show": "Show Reviews",
"header": "{total, plural, =0{0 Reviews} one {# Review} other {# Reviews}}",
"rating": "Rating",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Duplicate key.

<textarea name="contact_question" id="contact_question" rows="5" cols="50" class="form-input"></textarea>
</div>

{{#if forms.contact.recaptcha.enabled}}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved the field down after the question description field since it is a practice that it should be the last element before the submit.

@mcampa mcampa Mar 7, 2017

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.

do we need to wrap it in the if block?
I think forms.contact.recaptcha.markup will be empty when is disabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would like to keep the captcha enabled check in place irrespective of what is returned in the markup field.

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.

That's not how we do this kind of things in Stencil, we removed most of all boolean Blueprint variables because we can check agains the data

@junedkazi junedkazi changed the title [WIP] STENCIL-2934 Add recaptcha v2 support to cornerstone. STENCIL-2934 Add recaptcha v2 support to cornerstone. Mar 7, 2017
@junedkazi

Copy link
Copy Markdown
Contributor Author

@bigcommerce/stencil-team

@mcampa mcampa left a comment

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.

Need a changelog entry

<textarea name="contact_question" id="contact_question" rows="5" cols="50" class="form-input"></textarea>
</div>

{{#if forms.contact.recaptcha.enabled}}

@mcampa mcampa Mar 7, 2017

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.

do we need to wrap it in the if block?
I think forms.contact.recaptcha.markup will be empty when is disabled.

@junedkazi junedkazi left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need a change log entry for this. Also since this is going to get merged first can we also add a new line at the end of last entry since if you don't the last release title wraps up.

@nickdengler

Copy link
Copy Markdown
Contributor

💚 LGTM 👍

@mcampa mcampa left a comment

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.

👍

@mcampa mcampa merged commit 84fcc93 into bigcommerce:master Mar 10, 2017
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.

3 participants