Skip to content

Conversation

@eugenekasimov
Copy link
Contributor

@eugenekasimov eugenekasimov commented Nov 10, 2022

PR Summary:

In this PR I use Prettier plugin to format all liquid files for snippets.

The idea is to run the Prettier plugin on all liquid files in Dawn. After that all devs will start using the plugin for default. That will allow us to avoid seeing changes that were made by formatting in PRs.

Why are these changes introduced?

Fixes #2054 .

Visual impact on existing themes

No visual impact.

Testing steps/scenarios

There shouldn't be any issue, however I think it's a good idea to test visually at least those snippets that were affected.

Demo links

Checklist

@eugenekasimov eugenekasimov marked this pull request as ready for review November 10, 2022 19:16
@ludoboludo ludoboludo self-assigned this Nov 10, 2022
@ludoboludo ludoboludo self-requested a review November 10, 2022 21:54
@metamoni metamoni self-requested a review November 11, 2022 08:20
@metamoni
Copy link
Contributor

Note for reviewers: hiding whitespaces might make it a bit easier to review the changes.

Screenshot 2022-11-11 at 10 21 08

metamoni
metamoni previously approved these changes Nov 11, 2022
ludoboludo
ludoboludo previously approved these changes Nov 11, 2022
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

Looks good 👍

My comments were just general observations

{% if settings.blog_card_style == 'card' %} color-{{ settings.blog_card_color_scheme }} gradient{% endif %}
{% if settings.blog_card_style == 'card' and media_height == nil and article.image == empty or show_image == false %} ratio{% endif %}
"
style="--ratio-percent: {{ 1 | divided_by: ratio | times: 100 }}%;"
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird I wonder if we need this here since it's also declared on the next element 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed this too, I'm sure it was just oversight. We shouldn't need both if the styles aren't modifying the --ratio value within the card__inner scope specifically or something like that. Probably best to leave the outer most instance

{% if extend_height %} card--extend-height{% endif %}
{% if card_collection.featured_image == nil and card_style == 'card' %} ratio{% endif %}
"
style="--ratio-percent: {{ 1 | divided_by: ratio | times: 100 }}%;"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I find it interesting that we have it twice but nothing to action in this PR anyway 👍

Comment on lines 52 to +61
<img
srcset="{%- if article.image.src.width >= 165 -%}{{ article.image.src | image_url: width: 165 }} 165w,{%- endif -%}
srcset="
{%- if article.image.src.width >= 165 -%}{{ article.image.src | image_url: width: 165 }} 165w,{%- endif -%}
{%- if article.image.src.width >= 360 -%}{{ article.image.src | image_url: width: 360 }} 360w,{%- endif -%}
{%- if article.image.src.width >= 533 -%}{{ article.image.src | image_url: width: 533 }} 533w,{%- endif -%}
{%- if article.image.src.width >= 720 -%}{{ article.image.src | image_url: width: 720 }} 720w,{%- endif -%}
{%- if article.image.src.width >= 1000 -%}{{ article.image.src | image_url: width: 1000 }} 1000w,{%- endif -%}
{%- if article.image.src.width >= 1500 -%}{{ article.image.src | image_url: width: 1500 }} 1500w,{%- endif -%}
{{ article.image.src | image_url }} {{ article.image.src.width }}w"
{{ article.image.src | image_url }} {{ article.image.src.width }}w
"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do another round of PRs where we use our image_tag filter on all/most our images.

@eugenekasimov eugenekasimov dismissed stale reviews from ludoboludo and metamoni via 12a5ffa November 18, 2022 20:20
@eugenekasimov eugenekasimov merged commit 539af27 into main Nov 21, 2022
@eugenekasimov eugenekasimov deleted the prettier-all-liquid-snippets branch November 21, 2022 17:02
@naxorn
Copy link

naxorn commented Nov 21, 2022

Issue

Noticed that the nil values got switched to null by prettier. For example, in article-card.liquid:Line22 it got changed from:

if media_aspect_ratio != nil

to

if media_aspect_ratio != null

I looked back in other files, such as main-product.liquid, and the same thing happened.

Resolution

Despite null working (hidden feature?), nil should be preferred as that is what is referenced in Shopify's theme documentation, such as when you

@ludoboludo
Copy link
Contributor

@naxorn I had a similar concern and saw it referenced a few times in some of the object documentation (for example product).

Screenshot

Which made me think that it would be ok 🤔 But something to align on with @charlespwd to see if we should force this change from `nil` to `null` or potentially get the docs updated to account for it.

@naxorn
Copy link

naxorn commented Nov 21, 2022

Yeah, I also noticed that, but figured it was a display quirk because JSON only allows null. Not familiar with ruby, but on the liquid github page there is an expression.rb file that appears to change null literals to nil internally, so this appears to be why it works!

@charlespwd
Copy link
Contributor

@naxorn, yep that's exactly it. They are equivalent.

The idea behind the rule is we want consistency. While you can write both, you should only have one in the codebase. I wasn't sure which one to pick, polled the partners on the partner slack and the poll came back with favour towards null instead of nil.

Almost everyone only used one in favour of the other, not knowing that they are equivalent.

Presumably because the Venn diagram of JS & Liquid developers overlaps more than the Ruby & Liquid developers.

@ludoboludo
Copy link
Contributor

Good context to have, thank you both!

Rabter1 added a commit to Rabter1/dawn-icletta that referenced this pull request Nov 23, 2022
commit dc81806
Author: Ken Meleta <[email protected]>
Date:   Tue Nov 22 13:32:46 2022 -0700

    Add media sizing settings to featured product (Shopify#2114)

commit 9875aea
Author: Ken Meleta <[email protected]>
Date:   Tue Nov 22 13:17:55 2022 -0700

    Adjust constrain media values for quick add modal (Shopify#2113)

    * Adjust constrain media values for quickadd

    * Adjust offset logic

    * Minor formatting fix

commit 2eb8bf6
Author: Ludo <[email protected]>
Date:   Tue Nov 22 11:50:50 2022 -0500

    Add fixed sizes for some images to prevent errors (Shopify#2087)

    * remove the mention of sizes where unnecessary

    * remove another instance

    * add fixed size to the images in structured data

    * adjust size to 1920

    * edit logo size in structured data

    * remove conditional info

    * change value to match other logo size values

commit 2f9b9f3
Author: Ludo <[email protected]>
Date:   Mon Nov 21 15:44:24 2022 -0500

    Remove alt attribute in image_tag filter where unnecessary  (Shopify#2117)

    * Add fallback to alt attribute where image_tag filter is used

    * remove unnecessary use of alt attribute in image_tag filter

commit 251e5d9
Author: Ludo <[email protected]>
Date:   Mon Nov 21 12:18:44 2022 -0500

    Move share button into a snippet (Shopify#2123)

    * Move share button into a snippet

    * change the property name to context and add details in snippet

    * address review comments

    * edit props names and description

    * Address reviewers comment. Remove unused prop and edit existing ID

    * move script at the top

commit 539af27
Author: Eugene Kasimov <[email protected]>
Date:   Mon Nov 21 09:01:58 2022 -0800

    Prettier liquid files snippets. (Shopify#2115)

commit 503fdf8
Author: Eugene Kasimov <[email protected]>
Date:   Thu Nov 17 08:51:21 2022 -0800

    Remove noscript css import for product recommendations (Shopify#2101)

commit b2b01fd
Author: Eugene Kasimov <[email protected]>
Date:   Thu Nov 17 08:50:33 2022 -0800

    Change variables names (Shopify#2055)

commit cf05d0d
Author: Francisca Calabria Rubio <[email protected]>
Date:   Thu Nov 17 16:50:55 2022 +0100

    Select text input content on trapFocus (Shopify#2106)

    * Select text input content on trapFocus

    * PR fix

commit b86d1f3
Author: Ken Meleta <[email protected]>
Date:   Thu Nov 17 08:50:43 2022 -0700

    Change thumbnail media fit to "fill" by default (Shopify#2044)

    * Change thumbnail media fit to cover by default

    * Simplify and cleanup thumbnail fit approach

commit 8ab8e61
Author: Ludo <[email protected]>
Date:   Thu Nov 17 10:50:28 2022 -0500

    Tweak the image_url size to be the max value where necessary (Shopify#2110)

    * Tweak the image_url size to be the max value where necessary

    * fix sizing in the header.

commit c337301
Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
Date:   Thu Nov 17 10:49:52 2022 -0500

    Update 1 translation file (Shopify#2130)

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

commit bc8f433
Author: Ken Meleta <[email protected]>
Date:   Thu Nov 17 08:49:10 2022 -0700

    Add constrain media and media fit settings to product page (Shopify#2052)

    * Add constrain height setting

    * Add constrain scaling functionality

    * Add media fit setting and functionality

    * Quick add override and typo fixes

    * Update setting language and code cosmetics

    * Update 8 translation files

    * Update 5 translation files

    * Update 3 translation files

    * Update 3 translation files

    * Update 1 translation file

    * Add comment

    Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
@vfonic vfonic mentioned this pull request Jan 27, 2023
9 tasks
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
lost-end-found pushed a commit to lostendfound/librairiesanstitre-shopify that referenced this pull request Oct 8, 2025
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.

Prettier all liquid files

6 participants