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

Add Dynamic Content Regions to Cornerstone - MERC-2436 #1023

Merged
merged 1 commit into from
Jun 30, 2017

Conversation

tabayomi
Copy link

@tabayomi tabayomi commented Jun 15, 2017

What?

Cornerstone with Region tags added to default locations.

Tickets / Documentation

Add links to any relevant tickets and documentation.

Merge Checklist

  • Update the final names of the regions
  • Make sure MERC-2432 is merged

{{else}}
<div class="header-logo header-logo--{{theme_settings.logo-position}}">
{{> components/common/store-logo}}
</div>
{{/if}}

{{#if template_file '===' 'pages/cart'}}
Copy link
Contributor

@mcampa mcampa Jun 23, 2017

Choose a reason for hiding this comment

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

Please don't do this. Instead use the partial helper.

In pages/cart.html add

{{#partial "header"}}
  {{{region name="cart-page-banner"}}}
{{/partial}}

@tabayomi tabayomi force-pushed the corner-regions-for-MERC-2436 branch from 6e49060 to 418dc38 Compare June 26, 2017 22:45
@@ -16,13 +16,14 @@
<h1 class="header-logo header-logo--{{theme_settings.logo-position}}">
{{> components/common/store-logo}}
</h1>

{{{region name="homepage-banner"}}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets call this homepage_banner for consistency with the other names.

Choose a reason for hiding this comment

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

I thought we were going to use header_bottom for this.
https://docs.google.com/spreadsheets/d/1P4ncQzCrCeJJp8sXgFuAxWKuDqhvFeHmkuJ3b5MytmI/edit#gid=0

Can't you just move it out of the conditional block to include this region on every page?

@tabayomi tabayomi force-pushed the corner-regions-for-MERC-2436 branch from 418dc38 to c5a6eee Compare June 28, 2017 19:34
@@ -16,13 +16,14 @@
<h1 class="header-logo header-logo--{{theme_settings.logo-position}}">
{{> components/common/store-logo}}
</h1>

{{{region name="homepage_banner"}}}

Choose a reason for hiding this comment

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

I thought we were going to use header_bottom for this.
https://docs.google.com/spreadsheets/d/1P4ncQzCrCeJJp8sXgFuAxWKuDqhvFeHmkuJ3b5MytmI/edit#gid=0

Can't you just move it out of the conditional block to include this region on every page?

Copy link
Author

Choose a reason for hiding this comment

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

Good points. 👍 Done.

Choose a reason for hiding this comment

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

thanks!

@tabayomi tabayomi force-pushed the corner-regions-for-MERC-2436 branch from c5a6eee to 384ad60 Compare June 28, 2017 19:50
@tabayomi
Copy link
Author

@junedkazi @mjschock - Please take a look at this PR let me know if we're missing anything required for merge.

Copy link
Contributor

@mjschock mjschock left a comment

Choose a reason for hiding this comment

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

lgtm

@mjschock
Copy link
Contributor

@tabayomi - i forgot to mention - can you please add a changelog entry

@@ -21,8 +20,7 @@ <h1 class="header-logo header-logo--{{theme_settings.logo-position}}">
{{> components/common/store-logo}}
</div>
{{/if}}


{{{region name="header_bottom"}}}
Copy link
Contributor

@poweredbyanna poweredbyanna Jun 30, 2017

Choose a reason for hiding this comment

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

Just a quick question - how will we differentiate the cart page from the home page when targeting this single region?

Copy link
Author

Choose a reason for hiding this comment

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

By placement parameters. Placements require specifying a template file. We target a page with content based on both the region name AND the template file.

So in the case of the home page it would target "pages/home". For the cart page - "pages/cart".

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 sweet, thanks

@tabayomi tabayomi force-pushed the corner-regions-for-MERC-2436 branch 2 times, most recently from 731978c to 2527ee0 Compare June 30, 2017 00:36
@tabayomi tabayomi force-pushed the corner-regions-for-MERC-2436 branch from 2527ee0 to 2328e7a Compare June 30, 2017 00:47
@tabayomi tabayomi merged commit b9c43c6 into bigcommerce:master Jun 30, 2017
@minankmori
Copy link

Hello Guys,

Can anyone please let me know what exactly the use of this "Regions" tag on header and products page?
I can see it is generating a blank div tag on one of our client stores but can not trigger why and how it is generating.

@pedelman
Copy link
Contributor

@minankmori see #1193 for more details

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.

7 participants