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

Snackbar: Fix __unstableHTML support #57067

Closed
wants to merge 1 commit into from

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Dec 14, 2023

What?

Fixes: #56767

I added the exact same handling we have from Notice compoenent.

Testing Instructions

In dev tools run:

wp.data
	.dispatch( 'core/notices' )
	.createNotice(
		'warning',
		'<strong>Some</strong> message',
		{
			type: 'snackbar',
			isDismissible: true,
			__unstableHTML: true,
		}
	);

@ntsekouras ntsekouras added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Dec 14, 2023
@ntsekouras ntsekouras requested review from mirka and ciampo December 14, 2023 15:07
@ntsekouras ntsekouras self-assigned this Dec 14, 2023
Copy link

github-actions bot commented Dec 14, 2023

Flaky tests detected in eabea47.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7210753452
📝 Reported issues:

@mirka
Copy link
Member

mirka commented Dec 15, 2023

Can I have confirmation from somebody (@WordPress/gutenberg-core) that this is actually needed in Snackbar?

I'm asking because:

  1. Despite this __unstableHTML prop being present in the Storybook docs, it's more like a TypeScript mistake and we've never actually shipped this functionality in Snackbar. So although we currently aren't bound by a back-compat contract, we would be bound to it if were to ship it now. In this case we shouldn't prefix it with __unstable anymore, and we should document it properly, warning about its dangers.
  2. When we look at the original PR that adds __unstableHTML functionality to the Notice component (which our TS types erroneously pull from), they seem to be quite reluctant about it, saying that it's not ideal and is intended to be "compat-only".

I'm ok with adding it if it is indeed necessary, but if not, we can just fix the TS types.

@ntsekouras
Copy link
Contributor Author

ntsekouras commented Dec 15, 2023

Good questions!

it's more like a TypeScript mistake

I saw that, but I thought it was just an omission in the implementation and not a TS mistake.

I'll cc @mcsf .

@mcsf
Copy link
Contributor

mcsf commented Dec 15, 2023

Thanks for digging out the history; I had forgotten about those discussions from 2018. :)

With all the context laid out, I'm leaning towards not touching snackbars, even if that means consumers will face inconsistencies between them and regular notices.

I don't know if this begs for a proper element-interpolation solution to be brought to notices and snackbars — or if we should first discuss the purpose of notices, and thus what consumers ideally should and shouldn't be able to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notices - __unstableHTML works for default, but not in snackbar
3 participants