fix: Markup is visible in checkout error notices #6800
Conversation
rubikuserbot
requested review from
a team and
tarunvijwani
and removed request for
a team
|
The release ZIP for this PR is accessible via: |
|
Size Change: +121 B (0%) Total Size: 874 kB
ℹ️ View Unchanged
|
|
Hi @opr, do you mind giving me a hand here? I had a look online about using @wordpress/element to properly render HTML inside notices, but I'm still seeing |
What exactly are you doing to get this? Can you share some code? What is inside the object? |
This is what the object contains (print because copying the object is just too plain):
|
|
I don't know if something changed in the meantime, but those links I referred might be outdated. Looking at |
|
|
Without changing These are the props being passed for |
…r-notices-when-the-error-is-caused-by-a-php-syntax-error
…r-notices-when-the-error-is-caused-by-a-php-syntax-error
…r-notices-when-the-error-is-caused-by-a-php-syntax-error
…r-notices-when-the-error-is-caused-by-a-php-syntax-error
wavvves
added
status: needs review
type: bug
There was a problem hiding this comment.
Thanks @wavvves it's working for me now, the link works.
I see the second paragraph in this notice has some bottom-margin which makes the notice look weird. I'm not sure how we can solve that for every use case, we can remove the margin on the last p element, but what if another notice has a different element with margin on.
Moreover, I'm not really confident we should be using __unstableHTML solve this. That API is unstable and could change without notice, do you feel OK with using it?
Since the notices wrapper has padding set, I would say we should remove margin-top for the
What do you think?
It's not using it that bothers me, but the fact that it exists 😅. |
Yes, I think this is fine! Please would you implement it?
OK, thank you, I think your solution is fine then 😄 |
…r-notices-when-the-error-is-caused-by-a-php-syntax-error
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
Thanks @alexflorisca 😎 |
There was a problem hiding this comment.
Thanks @alexflorisca 😎 I think there's still a failing test caused by this (maybe just needs to be updated)
May I know which test is failing for you? It is working for me.
The unit tests all pass in CI so I think we're good there. The e2e tests I assume are down to flakiness but that might be a bad habit I got into! |
@tarunvijwani @alexflorisca It's this one that seems related to this. The Gutenberg ones are the flaky stuff, and there seems to be another (expecting /checkout and got /checkout-block) that I would put down my money got introduced because it slipped under the radar due to the flaky ones, right? |
Forget about that, I thought it was a notice, so unrelated. Still, the non-Gutenberg failing tests seem fixable. (that one expecting "Your cart is empty!" whereas the text seems to be "Your cart is currently empty!". I'll try a quick fix for that on this branch.) PS- It seems to have fixed that particular test. I think I'm merging this now and will leave any additional E2E investigation to the proper issue. I merging this :) |
…r-notices-when-the-error-is-caused-by-a-php-syntax-error
wavvves
deleted the
fix/6628-markup-is-visible-in-checkout-error-notices-when-the-error-is-caused-by-a-php-syntax-error
branch
|
Excluding from testing notes as discussed here: p1664204890072169-slack-C02UBB1EPEF |
* Enabled __unstableHTML hidden option for HTML rendering within notices. * Fixed margin-bottom for HTML notice content * Fixed margin-top for HTML notice content * Attempt to fix a broken e2e test
This PR fixes the displaying of unrendered HTML within our checkout notices.
Fixes #6628
Accessibility
prefers-reduced-motionOther Checks
Screenshots
Before:
After:
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog