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

STENCIL-2598 - Add GeoTrust SSL Seal Toggle #903

Merged
merged 3 commits into from
Feb 14, 2017
Merged

STENCIL-2598 - Add GeoTrust SSL Seal Toggle #903

merged 3 commits into from
Feb 14, 2017

Conversation

mjschock
Copy link
Contributor

@mjschock mjschock commented Jan 11, 2017

what (STENCIL-2598, formerly MERC-1160)

  • adds a toggle for the GeoTrust SSL Seal
  • implements the the instructions here
  • the domain and size can be set

@bigcommerce/stencil-team

@bigbot
Copy link

bigbot commented Jan 11, 2017

Autotagging @mcampa @bc-miko-ademagic @davidchin

@stencil-bot
Copy link

Generating bundle for commit dc07693

@stencil-bot
Copy link

Generating bundle for commit ac717b0

@stencil-bot
Copy link

Generating bundle for commit 8d99a41

Copy link
Contributor

@mcampa mcampa left a comment

Choose a reason for hiding this comment

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

👍

@mjschock mjschock changed the base branch from spraypaint to master January 12, 2017 21:53
<a href="http://www.geotrust.com/ssl/" target="_blank" style="color:#000000; text-decoration:none; font:bold 7px verdana,sans-serif; letter-spacing:.5px; text-align:center; margin:0px; padding:0px;"></a>
</td>
</tr>
</table>

Choose a reason for hiding this comment

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

Missing NL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm... yes, i see. the linter doesn't look at html or json. wondering if we should add something to do the check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've added a new line at the end but it doesn't seem to show up here, although you can see that there are now 9 lines: https://github.com/mjschock/stencil/blob/f80cdfc06862a9668db449697261c8d9a07eb4a2/templates/components/common/ssl-seal.html

config.json Outdated
@@ -590,4 +593,4 @@
}
}
]
}
}

Choose a reason for hiding this comment

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

Missing NL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@willfaught
Copy link

willfaught commented Jan 25, 2017

🍹 Keep in mind https://github.com/bigcommerce/standards/blob/master/docs/Git.md. Can we add the ticket (MERC-1160?) to the summary and use a verb (add?) and sentence case (Add GeoTrust SSL to do...)?

@mjschock mjschock changed the title geotrust ssl seal Add GeoTrust SSL Jan 25, 2017
@mjschock mjschock changed the title Add GeoTrust SSL STENCIL-2598 - Add GeoTrust SSL Seal Toggle Jan 25, 2017
.eslintrc Outdated
@@ -7,7 +7,7 @@
"complexity": 1,
"no-alert": 0,
"consistent-return": 0,
"max-len": 0,
"max-len": 0
Copy link
Contributor

@mcampa mcampa Jan 25, 2017

Choose a reason for hiding this comment

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

Unnecessary change. .eslintrc is not "real" json format 🍹 .
You can have trailing commas and add comments in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, just removed it because vscode doesn't like it. it's been added back :)

config.json Outdated
"social_icon_placement_bottom": "bottom_none"
"social_icon_placement_bottom": "bottom_none",
"show_geotrust_ssl_seal": false,
"geotrust_ssl_seal_hostname": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use shopPath instead ? Is there a reason for not using the store domain instead of offering a separate field ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question, however shopPath or ShopPathSSL return a url http://... not the hostname. But we could return the hostname in the Setting resource and use that

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have base_url & secure_base_url for both http & https links. We also have the home in the urls section of the context. All the urls are with http{s}:// so we will need to strip them out. The question is should we pass it as resource on the bcapp side or just add a helper in pencil to strip http{s}://.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 It would be useful to have helper for that. Like getHostname

But the question is if merchants really need to enter their own domain

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the trust ssl seal is for the store domain name. I don't see a reason why we should expose a field for the user to enter it unless there is some historic reason which I am not aware of.

@mcampa
Copy link
Contributor

mcampa commented Feb 7, 2017

We are now using CHANGELOG.md. Please add an entry with your change under draft before merging

## Draft
- <here>

## 1.5.1 (2017-02-07)
- ...

Copy link
Contributor

@junedkazi junedkazi left a comment

Choose a reason for hiding this comment

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

Changelog is missing for this PR

@@ -5,6 +5,7 @@
- Prevent carousel images from being cut off on large screens by adding a new setting to theme editor schema [#909](https://github.com/bigcommerce/stencil/pull/909)
- Add schema description specifying that social media icons must be set up to see them [#920](https://github.com/bigcommerce/stencil/pull/920)
- Show account creation links only if it is enabled in store settings [#917] (https://github.com/bigcommerce/stencil/pull/917)
- Add GeoTrust SSL Seal Toggle [#903] (https://github.com/bigcommerce/stencil/pull/903)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@junedkazi - updated

@junedkazi
Copy link
Contributor

junedkazi commented Feb 10, 2017

@nickdengler can we test this one before we merge it.

@nickdengler
Copy link
Contributor

LGTM 💚

@mjschock mjschock merged commit 8113b67 into bigcommerce:master Feb 14, 2017
@mjschock mjschock deleted the MERC-1160 branch February 14, 2017 19:41
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.

None yet

7 participants