Help notice displayed on Privacy Policy page missing in block editor

[aduth]

Previously: #45057

Related: ​https://github.com/WordPress/gutenberg/pull/13442

When a page is set to be the Privacy Policy page, a help notice containing a link to the Privacy Policy guide is displayed in the editor. This was originally fixed in #45057, but upon the expectation that Gutenberg would automatically upgrade notices output to the screen. This was later reverted:

​https://github.com/WordPress/gutenberg/pull/12444

Thus, the notice does not appear as expected when visiting the block editor.

It may need to be reimplemented instead using the "Notices" module implemented client-side.

​https://wordpress.org/gutenberg/handbook/designers-developers/developers/packages/packages-notices/
​https://wordpress.org/gutenberg/handbook/designers-developers/developers/data/data-core-notices/

[garrett-eclipse]

Displaced notice @ 960px wide

[garrett-eclipse]

Gap above notice on mobile

[garrett-eclipse]

Dismissible X overlaps notice contents

[garrett-eclipse]

Thanks @aduth I appreciate you taking a look at this.

The patch applies cleanly to trunk and introduces the notice to Gutenberg so that's a great start. Below, I provided some feedback for some issues and thoughts.

The Gutenberg Notice itself;

• At 960px wide the notice is displaced from the left. (screenshot)
• On mobile there's a gap between the admin bar and the notice. (screenshot)
• At various widths the dismiss close (x) overlaps the text. (screenshot)

*These issues may apply in general to the notices module, as you wrote it I assume you'd know better than I ;)

As the dismiss isn't persistent in that a reload will revive the notice we should remove the dismiss from this notice and address it along w/ the dismissal for the classic editor version in #44981.

I noticed this patch removes support of the notice when Classic Editor is enabled. We should continue providing the notice on the classic editor interface until at least official support for the classic editor ends.

Looking at the code, the switch to block editor notices also stripped the translator comment. This should be preserved, and if we have to implement two versions of the notice to reduce i18N we should stick with one string w/ translator comments. As the block editor notice supports the link string it should also get a unique translator comment and I suggest it be updated to 'View Privacy Policy Guide' to be explicit.

Thanks for the patch, let me know if I can clarify anything.

[aduth]

Thanks for the great feedback @garrett-eclipse !

*These issues may apply in general to the notices module, as you wrote it I assume you'd know better than I ;)

I think you'd be correct here, though I specifically recall several iterations back and forth with @joen in the initial implementation toward a few of these points (mobile specifically). I'd be curious if there's either been a regression or a mistake in the port to core.

As the dismiss isn't persistent in that a reload will revive the notice we should remove the dismiss from this notice and address it along w/ the dismissal for the classic editor version in #44981.

Yeah, I agree. I think it should also be a feature built-in to the notices module, and could probably leverage existing persistence functionality offered to data stores (of which notices extends) via the data persistence plugin.

​https://github.com/WordPress/gutenberg/tree/master/packages/data/src/plugins/persistence

I noticed this patch removes support of the notice when Classic Editor is enabled. We should continue providing the notice on the classic editor interface until at least official support for the classic editor ends.

Yeah, I wasn't sure what to do about this one, whether it ought to remain as-is or perhaps shift to be implemented specifically as part of the Classic Editor plugin.

Looking at the code, the switch to block editor notices also stripped the translator comment. This should be preserved, and if we have to implement two versions of the notice to reduce i18N we should stick with one string w/ translator comments. As the block editor notice supports the link string it should also get a unique translator comment and I suggest it be updated to 'View Privacy Policy Guide' to be explicit.

The problem here was that the notices module specifically does not support arbitrary markup in message texts†, including things like links. This needs to be enhanced in the notices module, perhaps as part of a general effort around allowing markup within translated texts for JavaScript strings.

​https://github.com/WordPress/gutenberg/issues/9846

† Technically it does offer support through the __unstableHTML property, but it's been repeatedly mentioned (even as recently as ​last week's editor meeting) that it's not something which is intended as a long-term solution.

[garrett-eclipse]

Thanks @aduth

I'll leave those notice module bugs in yours and @joen's trusted hands.

When looking at #44981 I'll take a look at the persistence plugin and see if that'll work for us. I am thinking when the Privacy Policy guide is updated that the notice should re-appear but am not sold on that.

As to the classic editor version, I feel it should be preserved in core until support ends as the Classic Editor plugin isn't the only one that will restore the classic editor functionality. There's also the 'Classic Editor Addon', 'Gutenberg Ramp', 'Disable Gutenberg', 'No Gutenberg' to name a few.
*As well the string is already available and translated as part of core.

As to the link in the verbiage, as we're following up with a link I feel we don't require the unstable HTML approach here. If we preserve the classic version too we can change the convention there so the link is separate from the string, this will allow a single string for both cases leaving only one to translate, along with the link.

[Joen]

I think you'd be correct here, though I specifically recall several iterations back and forth with @joen in the initial implementation toward a few of these points (mobile specifically). I'd be curious if there's either been a regression or a mistake in the port to core.

Thanks for the ping.

I have my eyes on these bugs, I'm pretty sure many of them there are open PRs for, but notably restoring the good stuff from ​https://github.com/WordPress/gutenberg/pull/12301#issuecomment-441572057 is on my todo list. That should address a number of these issues.

[Joen]

New PR that helps mitigate some of this ​https://github.com/WordPress/gutenberg/pull/13614

[garrett-eclipse]

Awesome @Joen took a look and it's slick. I like how dismissable notices are sticky.

[garrett-eclipse]

Updated patch to preserve the classic editor notice and apply the block editor notice without the dismissable.

[garrett-eclipse]

Block editor Privacy Policy guide notice

[garrett-eclipse]

Classic editor Privacy Policy guide notice

[garrett-eclipse]

Thanks @aduth and @Joen for your work on the block editor notices.

I've updated @aduth's original patch as follows;

• Preserved the classic editor notice.
• Updated the classic editor notice to use the same message, url and label as the block editor notice. This change moves the link after the message into a link. This link keeps the translator comments and the accessibility '(opens in a new tab)' screen reader text.
• Updates the block editor notice to use the same message, url and label as well as disabled the dismissable functionality.
• The label used in both cases is 'View Privacy Policy Guide.'
• The url used in both cases is escaped with esc_url

This accounts for the discussion feedback and avoids the need for __unstableHTML while keeping both versions of the notice consistent.

As well with the dismissable removed from the block editor notice the X no longer overlaps as well it doesn't stick so shouldn't overlap block editor toolbars.

Moving this into 5.2 for review and testing.

[xkon]

46098.2.diff​ adds some minor CS fixes & looks good while testing this on my end. Marking for commit.

[garrett-eclipse]

Thanks for catching the CS issues I've retested with your patch and everything functions properly both in the classic editor and the block editor. Cheers

[aduth]

Reviewed and tested the latest patch. Looks and works great from my end as well. I'll plan to commit shortly.

[aduth]

In 45174:

Privacy: Display help notice on block editor screen.

Use the Notices data module when viewing the block editor screen for the assigned Privacy Policy page to display a help notice.

Props garrett-eclipse, joen, xkon.
Fixes #46098.

[garrett-eclipse]

Thanks for the review and commit @aduth as well as the initial Block Editor patch. All the best

[garrett-eclipse]

An unnecessary string ended up coming from the classic notice, sorry. This should be static (non-translatable) and use %s as numeric placeholders aren't needed

[garrett-eclipse]

Patch to address the unnecessary string. Drops the translator comment, translation function and numeric placeholders are replaced with %s

[garrett-eclipse]

Hi @aduth now that these strings have perculated to translate.wp.org I realized that there was an unnecessary string that shouldn't be translatable.

I've uploaded 46098.3.diff to address this and remove the translation comment, string and make the placeholders %s as they'll never move.

Can you take a look so we can remove the unnecessary string.

Sorry

[aduth]

Ah, right you are. I'd missed it in my review. The updated patch looks good. I note the small difference of the space occurring outside the screen reader span where before it'd been contained within. In the browser, it appears to make no difference and the trailing whitespace is collapsed. As proposed appears to be the more common convention elsewhere in the codebase as well.

It occurs to me that the target attribute shouldn't need to be inserted using placeholder substitution. I suppose it was arguably more necessary when the link was translateable. Do you think it ought to be removed now? A similar point could be made about the screen reader text element, though there may be an argument in favor of general code legibility to keeping the substitution.

[garrett-eclipse]

Thanks @aduth

Yes, the move of the space outside of the span was to follow the convention as it felt odd to have it within the span.

And good call with the target and placeholder substitution. This looks good and 46098.4.diff applies nicely.

I'd say good to go.

Cheers

[aduth]

In 45180:

Privacy: Remove unnecessary translation from help notice action label.

As static markup, the Privacy Policy editor help notice link no longer needs to be translateable with the revisions included in [45174].

Props garrett-eclipse.
Fixes #46098. See [45174].