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

Description defaults to true for collection #577

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

tyleralsbury
Copy link
Contributor

@tyleralsbury tyleralsbury commented Sep 7, 2021

Why are these changes introduced?

Very quick PR to make the description on by default on the collection page. Discussed with @Oliviammarcello and Shawn Rudolph. This is good for SEO as well as for merchant expectations - they don't need to go to another place to see the collection description when they add one to their collection.

What approach did you take?

Switched a boolean 🤯

Checklist

Copy link
Contributor

@sofiamatulis sofiamatulis left a comment

Choose a reason for hiding this comment

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

Nice 🚢 Just added one comment

@@ -46,7 +46,7 @@
{
"type": "checkbox",
"id": "show_collection_description",
"default": false,
"default": true,
"label": "t:sections.main-collection-banner.settings.show_collection_description.label"
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that even if the setting is on, but the string is empty it still renders the div

https://screenshot.click/07-41-0tkji-x4y83.png

Should we also be checking the length of the content?

      {%- if section.settings.show_collection_description and  collection.description.length > 0 -%}
        <div class="collection-hero__description rte">{{ collection.description }}</div>
      {%- endif -%}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be fine, since we have div:empty { display: none }. This way, we can hide it without adding complexity to the Liquid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes 👍 I forgot about that. Thank you!

@sofiamatulis sofiamatulis self-assigned this Sep 7, 2021
@martinamarien martinamarien self-assigned this Sep 7, 2021
@martinamarien martinamarien merged commit a06cc25 into main Sep 7, 2021
@martinamarien martinamarien deleted the description-on-default branch September 7, 2021 19:59
@martinamarien martinamarien mentioned this pull request Sep 13, 2021
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
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