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-4566 Add option to hide breadcrumbs and page title #1444

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

kchu93
Copy link
Contributor

@kchu93 kchu93 commented Feb 13, 2019

What?

Add option to hide breadcrumbs, page heading, category heading, contact page heading and blog page heading.

Tickets / Documentation

MERC-4566

Turning off breadcrumbs
feb-14-2019 12-31-38

Turn off page heading
feb-14-2019 12-32-33

Turn off category page heading
feb-14-2019 12-34-04

Turn off contact page heading
contact

Turn off blog page heading
blog

Images

screen shot 2019-02-14 at 4 10 13 pm

@bigbot
Copy link

bigbot commented Feb 13, 2019

Autotagging @bigcommerce/storefront-team @davidchin

templates/components/common/breadcrumbs.html Outdated Show resolved Hide resolved
templates/pages/category.html Outdated Show resolved Hide resolved
@Tiggerito
Copy link
Contributor

I think the terminology used here is confusing. Technically these are not page titles, but headings. In admin they are generally called page names.

And the Search Engine Optimisation sections use the term page title (correctly) for the title tag, which is a different thing.

I suggest you use "Hide page heading".

@kchu93 kchu93 force-pushed the MERC-4566 branch 3 times, most recently from eceeb54 to e3e3834 Compare February 14, 2019 20:27
@kchu93
Copy link
Contributor Author

kchu93 commented Feb 14, 2019

After speaking with design, we want to be very clear with which page headings we want to turn off. I have added individual options (type 'page' heading, category heading, contact us heading, and blog heading)

@mattcoy-arcticleaf
Copy link
Contributor

mattcoy-arcticleaf commented Feb 20, 2019

The names (id's) of the settings themselves are a bit confusing. I would think that a setting with an id of "breadcrumbs" would enable them if set to true, rather than the opposite being true, which is the case here. I would recommend changing the names to "hide_breadcrumbs", "hide_page_heading", etc.

From a code perspective, since you are only checking for a single item in the conditional statements, you do not need to use the "or" helper. A simple {{#if ../theme_settings.blog_page_heading '!==' true}}, would suffice. Also, since you are checking for an inverse, this can be written with even less code using an {{#unless}} helper. Finally, since the scope has not changed in these files (except in the case of the breadcrumbs where you put it inside the each loop), you do not need to add the '../', leaving the final code something like: {{#unless theme_settings.blog_page_heading }}code here{{/unless}}.

Note that in the case of the breadcrumbs, you would need to use the ../, or move the conditional outside the each block.

@kchu93
Copy link
Contributor Author

kchu93 commented Feb 20, 2019

@mattcoy-arcticleaf Thank you so much for the feedback, this was super helpful!

@Ubersmake Ubersmake merged commit e9c925a into bigcommerce:master Feb 28, 2019
davidwrpayne pushed a commit that referenced this pull request Mar 13, 2019
Update Changelog to include Merged PR #1444
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