-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Checkout summary: avoid carriage return on price #11105
Checkout summary: avoid carriage return on price #11105
Conversation
3173987
to
5c8e5d1
Compare
5c8e5d1
to
cd18e35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because someone accidentally entered a carriage return in the system config? Perhaps we should prevent that with validation (or simply strip it in the text input with pattern=""
.
This certainly solves it with the current data though 👍
Also I was curious to discover that we display prices with all the extra markup. And found that it's not on the shopfront or cart, only in the checkout summary.
Do you know why it's there and if it's necessary?
Nop, it's because how the browser react to DOM.
I noticed that also, but no... I'm not sure why it's used here and not in other places (as it should be I suppose) |
Ohhh.. I see now, it's because there are spaces in the currency config, and the narrow columns caused it to wrap. Good fix 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one !
Hi @jibees, |
Hey @drummer83 @jibees The failing spec looks like: So maybe we're good to test? Any clue why failing looks more persistent on this branch @jibees ? |
cd18e35
to
1cf647e
Compare
Nop, locally, everything is fine I've rebased the PR, and error is still present, and I think it's probably relevant. I should investigate, but I have no clue! |
This is definitely flaky, and now it's green... |
Hi @jibees, I can confirm that the /summary step of split checkout is fixed. I tested this for different screen sizes as well. 👍 We still see this issue for the order TOTAL on the /cart page and for payment and shipping fees (step 1 and 2 of split checkout) for some screen sizes. But this is out of scope here. I think we are safe to merge this one. Thanks! 🙏 |
Yes, unfortunately, we don't use |
What? Why?
What should we test?
Release notes
Changelog Category: User facing changes
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates