add currency code to cart and checkout block totals when mccy is enabled #8636
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reykjalin
requested review from
a team and
cesarcosta99
and removed request for
a team
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +109 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
…icit-currency-code
cesarcosta99
requested changes
Apr 15, 2024
There was a problem hiding this comment.
Code looks good and tests well! But I ran into something interesting while testing this.
- Make sure WC 8.7 and/or 8.6 are still properly supported ✅
- I tested it on both versions and everything seems to be ok 🎉
- Make sure currency code is displayed when using WC 8.8
⚠️ - I tested with USD and EUR and everything worked as expected, except one thing: while using EUR the formatting of the currency didn't match the shortcode version. Any idea why? This issue also triggered me a thought about whether it'd be feasible to perform this change in the backend rather than in the frontend 🤔
| Shortcode version | Blocks version |
|---|---|
|
|
|
Thanks for the review @cesarcosta99!
I have no idea why. And the shortcode version is hilariously wrong about the formatting. The correct formatting for EUR is €, definitely not €. There has to be some bug somewhere causing this, but I don't have a clue what it is. I even have my store currency set to USD with the correct USD formatting, so it's not like the store currency formatting is messing with things. I can, however, say with confidence that it's not this PR. The filter I'm using in the blocks expects a string returned with a This definitely warrants an issue though, nice catch! I'll open one 👍
Not to my knowledge, no. AFAICT the Cart and Checkout blocks apply their own formatting based on configurations available in the front-end. They don't receive a formatted amount from the backend 😕 . |
|
I have just learned that both formats are valid 😮 . So maybe this is a consistency bug, but it's not introduced in this version. I went back to 4d46126 (middle-ish of March this year) and it was still present there. |
…icit-currency-code
|
@cesarcosta99 FYI I've enabled merge when ready 👍 |
cesarcosta99
approved these changes
Apr 17, 2024
There was a problem hiding this comment.
I have just learned that both formats are valid 😮 . So maybe this is a consistency bug, but it's not introduced in this version
Yeah, I didn't see anything wrong in the code itself as you do <price/> ${ cart.cartTotals.currency_code }. This issue is somewhat unrelated to your changes, but I still wanted to ask to see if anything popped up.
Thanks for creating an issue for this 💯 
reykjalin
deleted the
fix/total-amounts-not-displayed-with-explicit-currency-code
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2648
Changes proposed in this Pull Request
Screenshots
Testing instructions
Note
This feature uses a blocks filter introduced in WC v8.8 which has not yet been released. See testing instructions for details.
Make sure WC 8.7 and/or 8.6 are still properly supported
develop).develop).Make sure currency code is displayed when using WC 8.8
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge