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

Use new asset filter #3582

Merged
merged 31 commits into from
Aug 27, 2024
Merged

Use new asset filter #3582

merged 31 commits into from
Aug 27, 2024

Conversation

tyleralsbury
Copy link
Contributor

@tyleralsbury tyleralsbury commented Aug 7, 2024

Refactor of SVGs in Dawn to use the new inline_asset_content Liquid filter.

sections/cart-icon-bubble.liquid Outdated Show resolved Hide resolved
sections/header.liquid Outdated Show resolved Hide resolved
sections/main-account.liquid Outdated Show resolved Hide resolved
sections/main-account.liquid Outdated Show resolved Hide resolved
sections/main-activate-account.liquid Show resolved Hide resolved
snippets/pagination.liquid Outdated Show resolved Hide resolved
snippets/product-media.liquid Outdated Show resolved Hide resolved
snippets/product-media.liquid Outdated Show resolved Hide resolved
snippets/product-thumbnail.liquid Outdated Show resolved Hide resolved
snippets/product-thumbnail.liquid Outdated Show resolved Hide resolved
@thagxt
Copy link

thagxt commented Aug 10, 2024

The time to introduce support for sub-directories in /assets/ has arrived.

assets/square.svg Outdated Show resolved Hide resolved
@melissaperreault
Copy link
Contributor

melissaperreault commented Aug 14, 2024

Found a wild bug, when you search in your Country/region selector, the input from the Search page are linked. This is in production though...

14-59-dr7a1-75s0h.mp4

@tyleralsbury
Copy link
Contributor Author

Found a wild bug, when you search in your Country/region selector, the input from the Search page are linked. This is in production though...

14-59-dr7a1-75s0h.mp4

For that one, you can create a separate issue. Weird one.

* Change width and height to be handled by parent context

* Optimized SVG files with svgo
@melissaperreault
Copy link
Contributor

melissaperreault commented Aug 22, 2024

Country/Region selector

assets/square.svg Outdated Show resolved Hide resolved
@bakura10
Copy link

Is the inline_asset_content already in production Liquid? I tried to check how it works but it does not seem to work.

Remainder for the dev team as well: make sure to not forget app blocks and make it sure it works for assets inside an app block.

@tyleralsbury
Copy link
Contributor Author

Is the inline_asset_content already in production Liquid? I tried to check how it works but it does not seem to work.

Remainder for the dev team as well: make sure to not forget app blocks and make it sure it works for assets inside an app block.

Not yet. Keep an eye out, though.

@Roi-Arthur
Copy link
Contributor

Roi-Arthur commented Aug 27, 2024

When using collection filters, the last checkbox seems slightly clipped. This seem to only be true if there are more than 2 options

On the password page, the Shopify icon in the footer is too small

Related but not new:
The localization search field escape button is every so slightly off center, or technically the field input has an unnecessary 1px margin

Unsure if this is directly linked to the recent changes since it shows up in the current version of Dawn, but some of the placeholder svg could use some extra CSS (height: auto)

Social media icons container could maybe use a look-over; currently the icon "spill out" of it, meaning the ones of the edges somewhat overlap with blocks on the sides

In the cart, the remove item icon isn't aligned

@tyleralsbury
Copy link
Contributor Author

👀 Found something very odd, some icons do not render on my iPhone, Safari browser 17.6.1. (Example 1, Example 2)

General

  • Quantity inputs' minus and plus icon
  • Login icon
  • Password page lock icon

Cart

  • Close X on Cart Drawer

Default product

  • Icons from the Icon with text block from Product template
  • Icons from the Collapsible row block
  • Icon for the Complementary products block visible when collapsible row is displayed as a setting

Default collection

  • Filter icon
  • Filter pill X remove icon
  • Filter drawer closing X icon

Good ol' Safari, always up for a surprise. Fixed it - TIL height and width auto don't work properly on Safari SVGs. 🤷

@melissaperreault
Copy link
Contributor

melissaperreault commented Aug 27, 2024

A few regressions to note while fixing for Safari:

  • Checkmark in Select is not vertically aligned (Visual)
  • Filter chevron not in place (Visual)
  • Complementary products block, when collapsible row turned on chevron is shifted (Visual)
  • Collection block arrow misaligned, to check in different section and template (Visual)

@tyleralsbury
Copy link
Contributor Author

Complementary products block on Product information

  • When Quick add is enabled; Icons are not accurate between Choose options and Add to cart
  • The Plus icon for Add to cart rotates, but not the appropriate way
  • Pagination numbers are not centered between the nav arrows (Visual)

26-03-x2wka-sufej.mp4

This set was a bit weird - I didn't see these issues so it's possible another batch of fixes already fixed them.

@melissaperreault
Copy link
Contributor

melissaperreault commented Aug 27, 2024

Complementary products block

  • Size of the arrow is large
  • We can see the pagination being off centered on the visual reference
Expected Current

@melissaperreault
Copy link
Contributor

melissaperreault commented Aug 27, 2024

Gift card product

  • Position of the checkbox label
  • Checkmark is misplaced and large
  • Error icon; Size of it and space between the icon and its label is wider
Expected Current

@melissaperreault
Copy link
Contributor

melissaperreault commented Aug 27, 2024

Might be coming from the Safari fix

  • Drawer nav arrows are large compared to main (Visual)

Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

Thank you Tyler and team for the hard work picking on the fine details. I am ready to put my approval on this! Let's continue to keep an eye on the other themes! 💪 🎉

@Thibaut
Copy link
Contributor

Thibaut commented Aug 27, 2024

Congrats 👏

Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

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

That's a huge amount of work 🚀

@tyleralsbury tyleralsbury merged commit 755f6a4 into main Aug 27, 2024
6 of 8 checks passed
@tyleralsbury tyleralsbury deleted the use-new-asset-filter branch August 27, 2024 17:58
@octipus
Copy link

octipus commented Aug 29, 2024

excuse me, where is this new filter documented?

@tyleralsbury
Copy link
Contributor Author

excuse me, where is this new filter documented?

The filter is not yet documented, but will be soon.

The usage is as seen in this Pull Request:

  1. Add the file into the /assets folder. Here we use it only for SVGs, but you can use other file types as well
  2. In your Liquid {{ 'file-name.ext' | inline_asset_content }}

And that's all there is to it - it will inline the content of the file into the document. Note that there is a filesize limit of 15kb for assets inlined by the filter.

@tyleralsbury
Copy link
Contributor Author

The docs are now available.

https://shopify.dev/docs/api/liquid/filters/inline_asset_content

@bakura10
Copy link

bakura10 commented Sep 3, 2024

Hi @tyleralsbury ,

At Maestrooo our icon snippet is a bit more complex. I've tried to do some experiment around this new filter but for now we unfortunately can't easily use it.

Would it be possible to add the class and style attribute to the inline_asset_content? The use case we have is that we support RTL in our themes (so we need a class for some icons to be able to flip them in RTL context). To limit the number of one-off class we also allow passing width as inline style, so that our end SVG often look like this:

<svg class="icon" style="--icon-width: 32px; --icon-mobile-width: 24px">
  ...
</svg>

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
* Moved snippets to assets and changed to svg
* Changed snippet icon rendering to use new inline_asset_content filter
* Removed hardcoded SVGs
* Made SVGs valid
* Refactored + Optimized svg files (Shopify#3586)

---------

Co-authored-by: Ludo <[email protected]>
Co-authored-by: melissaperreault <[email protected]>
@montalvomiguelo
Copy link

montalvomiguelo commented Sep 4, 2024

There is an alternate approach to managing icons with SVG symbols, which is my favorite one, and I use it for my Tailwind projects. The icons could live in assets/icons.svg instead of snippets/icons.liquid. Then they can be referenced via <use> tag.

<svg class="some essential classes">
  <use href="{{ icons.svg | asset_url }}#icon-arrow"/>
</svg>

Since assets now load from the same origin, this approach works nicely. For theme editor could use svgxuse.

@tyleralsbury
Copy link
Contributor Author

Hi @tyleralsbury ,

At Maestrooo our icon snippet is a bit more complex. I've tried to do some experiment around this new filter but for now we unfortunately can't easily use it.

Would it be possible to add the class and style attribute to the inline_asset_content? The use case we have is that we support RTL in our themes (so we need a class for some icons to be able to flip them in RTL context). To limit the number of one-off class we also allow passing width as inline style, so that our end SVG often look like this:

<svg class="icon" style="--icon-width: 32px; --icon-mobile-width: 24px">
  ...
</svg>

Since inline_asset_content can output the value of any asset type, it would be difficult to map things like class or style to its output (think a CSS or JS file, for example). What you are suggesting here sounds more appropriate for a more SVG-specific filter of some sort. What do you think?

@bakura10
Copy link

bakura10 commented Sep 5, 2024

Thanks @tyleralsbury ! That makes sense, I understand there is also probably some heavy caching involved to be able to inline the content and that customizing the output might not be possible.

Having said that, having parameters dependant of the type of resource is not a first. The metafield_tag and metafield_text for instance have some parameters that are dependant of the type of metafield being passed (such as the field being only used for metaobject). As soon as the behavior is documented it should be ok. I would prefer that rather than a new filter.

NikoBerger pushed a commit to NikoBerger/palmafits-dawn that referenced this pull request Sep 26, 2024
* Moved snippets to assets and changed to svg
* Changed snippet icon rendering to use new inline_asset_content filter
* Removed hardcoded SVGs
* Made SVGs valid
* Refactored + Optimized svg files (Shopify#3586)

---------

Co-authored-by: Ludo <[email protected]>
Co-authored-by: melissaperreault <[email protected]>
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.