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

MERC-4842 Add additional regions #1491

Merged
merged 1 commit into from
Jun 4, 2019
Merged

Conversation

kchu93
Copy link
Contributor

@kchu93 kchu93 commented May 7, 2019

What?

Enable the following regions in cornerstone for enabling adding new content via widgets onto these pages:
(1) Homepage
(2) Brand
(3) Brands
(4) Category
(5) Product Page
(6) Search Page

Tickets / Documentation

MERC-4842

@bigbot
Copy link

bigbot commented May 7, 2019

Autotagging @bigcommerce/storefront-team @davidchin

@kchu93 kchu93 force-pushed the MERC-4842 branch 2 times, most recently from 045206e to 38cfbfc Compare May 7, 2019 21:01
templates/pages/home.html Outdated Show resolved Hide resolved
templates/pages/home.html Outdated Show resolved Hide resolved
templates/pages/home.html Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented May 8, 2019

@kchu93 did you include removing the snippets or that was done as part of a separate PR?

@kchu93 kchu93 force-pushed the MERC-4842 branch 3 times, most recently from 2d76ee1 to dcce76a Compare May 17, 2019 17:39
@kchu93
Copy link
Contributor Author

kchu93 commented May 31, 2019

@bookernath @BC-EChristensen So for this PR we have removed the page_content region until discussion has been finalized, but aside from that, all other regions are ready to be merged! Thanks again for all the help / review on this

<div class="navPages-container" id="menu" data-menu>
{{> components/common/navigation-menu}}
</div>
</header>
{{{region name="header_bottom"}}}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is header bottom shouldn't this be inside the header tag ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i spoke with @pedelman / @BC-EChristensen and i believe they wanted to move this outside of the header tag due to its placement. I can move this back if necessary, but they may be able to speak on this as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want you to move ... I am just trying to understand the naming and the reasoning behind it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately we already have two regions, header_bottom and product_below_price, using two different naming schemes. We opted to adopt the second, but unfortunately we can't change this one.

You're right about menu_bottom below, though, we're changing that to home_below_menu

@@ -21,6 +21,7 @@
</div>
{{/if}}
<h1 class="page-heading">{{brand.name}}</h1>
{{{region name="brand_below_header"}}}
Copy link
Contributor

@junedkazi junedkazi May 31, 2019

Choose a reason for hiding this comment

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

Can you please explain to me the logic behind when to use bottom and when to use below ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Juned, similar to what Erik was stating above, but I believe we opted for below / 'above' as it would specify more so where a region would be.

@@ -12,9 +12,11 @@
limit: {{theme_settings.homepage_blog_posts_count}}
---
{{#partial "hero"}}
{{{region name="home_below_menu"}}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed menu_bottom to home_below_menu

@christensenep
Copy link
Contributor

@junedkazi Kevin made some changes to the naming to be more consistent. We can't change header_bottom unfortunately, though. Mind taking another look at this?

@kchu93 kchu93 merged commit 27c57cf into bigcommerce:master Jun 4, 2019
@kchu93 kchu93 deleted the MERC-4842 branch June 4, 2019 20:20
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

4 participants