WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 7 months ago

#46098 closed defect (bug) (fixed)

Help notice displayed on Privacy Policy page missing in block editor

Reported by: aduth Owned by: garrett-eclipse
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.0
Component: Privacy Keywords: has-patch has-screenshots commit
Focuses: administration Cc:
PR Number:

Description

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/

Attachments (11)

46098-pp-notice-js.diff (2.7 KB) - added by aduth 10 months ago.
Screen Shot 2019-01-26 at 6.48.04 PM.png (465.4 KB) - added by garrett-eclipse 10 months ago.
Displaced notice @ 960px wide
Screen Shot 2019-01-26 at 6.52.16 PM.png (331.8 KB) - added by garrett-eclipse 10 months ago.
Gap above notice on mobile
Screen Shot 2019-01-26 at 6.52.02 PM.png (712.4 KB) - added by garrett-eclipse 10 months ago.
Dismissible X overlaps notice contents
46098.diff (2.5 KB) - added by garrett-eclipse 8 months ago.
Updated patch to preserve the classic editor notice and apply the block editor notice without the dismissable.
Screen Shot 2019-03-25 at 12.02.03 AM.png (317.6 KB) - added by garrett-eclipse 8 months ago.
Block editor Privacy Policy guide notice
Screen Shot 2019-03-25 at 12.02.47 AM.png (456.9 KB) - added by garrett-eclipse 8 months ago.
Classic editor Privacy Policy guide notice
46098.2.diff (2.8 KB) - added by xkon 7 months ago.
Screen Shot 2019-04-12 at 12.58.18 PM.png (170.3 KB) - added by garrett-eclipse 7 months ago.
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
46098.3.diff (743 bytes) - added by garrett-eclipse 7 months ago.
Patch to address the unnecessary string. Drops the translator comment, translation function and numeric placeholders are replaced with %s
46098.4.diff (848 bytes) - added by aduth 7 months ago.

Change History (35)

#1 @aduth
10 months ago

  • Keywords has-patch added

@garrett-eclipse
10 months ago

Displaced notice @ 960px wide

@garrett-eclipse
10 months ago

Gap above notice on mobile

@garrett-eclipse
10 months ago

Dismissible X overlaps notice contents

#2 @garrett-eclipse
10 months ago

  • Focuses administration added
  • Keywords needs-refresh added
  • Version set to 5.0

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.

#3 @garrett-eclipse
10 months ago

  • Milestone changed from Awaiting Review to Future Release

#4 @aduth
10 months ago

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.

#5 @garrett-eclipse
10 months ago

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.

#6 @Joen
10 months ago

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.

#7 @Joen
10 months ago

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

#8 @garrett-eclipse
10 months ago

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

@garrett-eclipse
8 months ago

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

@garrett-eclipse
8 months ago

Block editor Privacy Policy guide notice

@garrett-eclipse
8 months ago

Classic editor Privacy Policy guide notice

#9 @garrett-eclipse
8 months ago

  • Keywords needs-testing added; needs-refresh removed
  • Milestone changed from Future Release to 5.2
  • Owner set to garrett-eclipse
  • Status changed from new to assigned

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.

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


8 months ago

This ticket was mentioned in Slack in #core-privacy by garrett-eclipse. View the logs.


8 months ago

#13 @garrett-eclipse
8 months ago

  • Keywords has-screenshots added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 months ago

@xkon
7 months ago

#15 @xkon
7 months ago

  • Keywords commit added; needs-testing removed

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

#16 @garrett-eclipse
7 months ago

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

#17 @aduth
7 months ago

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

#18 @aduth
7 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#19 @garrett-eclipse
7 months ago

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

@garrett-eclipse
7 months ago

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
7 months ago

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

#20 @garrett-eclipse
7 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

This ticket was mentioned in Slack in #core by desrosj. View the logs.


7 months ago

#22 @aduth
7 months ago

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.

@aduth
7 months ago

#23 @garrett-eclipse
7 months ago

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

#24 @aduth
7 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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].

Note: See TracTickets for help on using tickets.