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

Add liquid to assign uses_comma_decimals on mobile #2980

Merged

Conversation

Roi-Arthur
Copy link
Contributor

PR Summary:

Adding a liquid check bring mobile facets filters to parity with desktop filters. In their current state they break (at least visually) when using a currency that uses commas as decimal separator.

Another small addition is the removal of the Turkish Lyra (TRY) from the list of such currencies, as Turkish Lyra does not in fact use a comma for decimals.
Edit: This is already being requested here

Why are these changes introduced?

Fixes #0.

What approach did you take?

Duplicated the existing liquid block that can be found around line 305 of the facets.liquid file and added it under the second instance of {% when 'price_range' %}
I debated adding the check further up to have it in the code only once but this approach ensures we only run that check when there is a price range filter.

Other considerations

Visual impact on existing themes

No impact other than fixing value of filters inputs

Testing steps/scenarios

  1. Create a product with a price higher than 1000
  2. Make sure your store uses a currency that uses commas as decimal separator (Euro for example).
  3. Make sure that price is an available filter (use Search & discovery app to set that up)
  4. In any of our themes, go to a collection that includes your product priced >1000
  5. Set your filter layout to "Drawer"
  6. Inspect the inputs for the price range filter and notice the issue.

Demo links

Uploading Drawer input max error fix.mp4…

Checklist

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 👍

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.

👍

@Roi-Arthur Roi-Arthur merged commit 2663ae1 into main Sep 6, 2023
@Roi-Arthur Roi-Arthur deleted the fix-facets-placeholder-max-decimal-separator-currencies branch September 6, 2023 13:45
AshEastham-idhl added a commit to AshEastham-idhl/dawn that referenced this pull request Sep 11, 2023
Add liquid to assign uses_comma_decimals on mobile (Shopify#2980)
@willbroderick
Copy link

This may not be the best place for a comment, but I wanted to mention we've used this in our themes for a while without problems:

  assign uses_comma_decimals = false
  assign currency_test_string = 101 | money_without_currency
  if currency_test_string == '1,01'
  	assign uses_comma_decimals = true
  endif

Testing the current money format, instead of baking in a list of currencies, seems a lot simpler and safer.

@ludoboludo
Copy link
Contributor

Thanks for sharing @willbroderick !

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.

4 participants