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

Clarify explanation of how 'Convert to Links' works in Page List block #45394

Merged

Conversation

artemiomorales
Copy link
Contributor

@artemiomorales artemiomorales commented Oct 28, 2022

What?

Fixes #44615.
This pull request clarifies the copy for the Convert to Links functionality in the Page List block, explaining where the current menu comes from and providing a link to the documentation, as per this issue.

Why?

This will make the functionality clearer to users and cut down on confusion.

How?

It's just a simple update of the copy.

Testing Instructions

  1. Select a Page List block in Full Site Editing mode
  2. Click the 'edit' button

Screenshot

Screen Shot 2022-10-28 at 3 33 01 PM

@codesandbox
Copy link

codesandbox bot commented Oct 28, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Oct 28, 2022
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @artemiomorales! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@artemiomorales artemiomorales added [Type] Enhancement A suggestion for improvement. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Block] Navigation Affects the Navigation Block labels Oct 28, 2022
<a href="https://wordpress.org/support/article/navigation-block/">
{ __( 'Click here' ) }
</a>
{ __( ' to learn more about menu management.' ) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't split and concatenate translated strings because they won't translate properly. instead we need to use the translation helper createInterpolateElement so that the entire chunk can be translated as a whole unit.

{ createInterpolateElement(
	__( '<a>Click here</a> to learn more about menu management.' ),
	{ a: <a href="https://wordpress.org/support/article/navigation-block" />
) }

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the wording updates here but I don't feel confident in approving the changes; please defer to what others say.

We do need to make sure though that we aren't splitting and concatenating translated strings, so once that's resolved I'll remove my block on this.

@artemiomorales
Copy link
Contributor Author

Thanks for checking this @dmsnell! The splitting of the translated string has been removed.

@dmsnell dmsnell self-requested a review October 29, 2022 16:27
@dmsnell
Copy link
Member

dmsnell commented Oct 29, 2022

Looks better @artemiomorales - just another poke though, it was fine by me to have "Click here" be the link; the only necessity was using the right translation tools to ensure it didn't break in other languages. 👍

@artemiomorales
Copy link
Contributor Author

@dmsnell Got it. Do you want the a tag text to be restricted to 'Click here' like it was previously before you approve this?

So this is my first time doing i18n, and I looked at internationalizing JavaScript in the handbook, as well as How to Internationalize Your Plugin, Internationalization and @wordpress/i18n.

I'm not sure how I'd go about doing this, but would consider using dangerouslySetInnerHTML, using translation files, or perhaps using wp_localize_script(), all of which seem like overkill when we can just set the link to the whole line, which would be clearer maintenance-wise and probably be a better UX overall.

Let me know your thoughts!

@dmsnell
Copy link
Member

dmsnell commented Oct 30, 2022

Do you want the a tag text to be restricted to 'Click here' like it was previously before you approve this?

I don't have any feelings on it; I simply responded because I saw you changed more than I had asked for, and I want you to be empowered to make the changes you want without running into the risks.

I'm not sure how I'd go about doing this

Did you try the code I included as an example? That should produce a new React component you can embed like any other.

const HelpLink = createInterpolateElement();

return <div><HelpLink /></div>;

@artemiomorales
Copy link
Contributor Author

@dmsnell

Did you try #45394 (comment)? That should produce a new React component you can embed like any other.

Apologies! I somehow missed that comment. Thank you for providing this snippet; that makes everything much clearer and is good info for the future. I also have no strong preference either way and decided to just go ahead and commit your suggestion. Much appreciated 🙏

@artemiomorales
Copy link
Contributor Author

Worth noting though: Using createInterpolateElement as suggested causes the linter to fail. I overrode that on my local, but it looks like the checks are also performed here on Github and failed as well, so not sure if that's an nonissue, if there's an alternative syntax that would be better to use, or if omitting createInterpolateElement would be best.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking this one. Very much appreciated.

I wonder if @jasmussen has a view on the wording here?

</p>
<p>
{ createInterpolateElement(
__( '<a>Click here</a> to learn more about menu management.' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid "Click here" as the link. This isn't good for a11y.

The wording should/could be:

Learn more about menu management.

And the whole string could be the link. That way if an assistive tech device encounters the link when browsing the page by hyperlinks it [the link] will make sense without additional context.

<p>
{ createInterpolateElement(
__( '<a>Click here</a> to learn more about menu management.' ),
{ a: <a href="https://wordpress.org/support/article/navigation-block" /> }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we make this open in a new tab?

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the updates, there is clearly an improvement over the dry text we have now, but I have a few nits in the comments.

) }
</p>
<p>
{ __(
"Note: if you add new pages to your site, you'll need to add them to your navigation menu."
'If you previously created a menu in a classic theme, converting will allow you to import it.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct! You can import classic menus from the menu selector in the inspector even if your current menu is a page list or contains a page list. Or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The experience is that you need to convert to links or create new in order to import a classic menu.

Screen Shot 2022-10-31 at 10 07 31

We might like to offer the import feature in the sidebar in all circumstances.

Also remember that the auto-import of classic menus is now in the Gutenberg Plugin so if you have an existing classic menu it will automatically be used and you will never see the page list.

</p>
<p>
{ createInterpolateElement(
__( '<a>Click here</a> to learn more about menu management.' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this addition!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cold not agree more. Providing inline support is key.

I have dropped a comment in the docs tracker to ensure the support article is up to date. WordPress/Documentation-Issue-Tracker#297 (comment)

className={ 'wp-block-page-list-modal' }
aria={ { describedby: 'wp-block-page-list-modal__description' } }
>
<p id={ 'wp-block-page-list-modal__description' }>
{ __(
'To edit this navigation menu, convert it to single page links. This allows you to add, re-order, remove items, or edit their labels.'
'We’ve provided a default list of your published pages so you can get started quickly. If you’d prefer to edit this menu, click the Convert button below — converting will allow you to add, re-order, and remove items, or edit their labels.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Who is "we"? Do we have other places with this form of addressing in WP?
  2. Coule the disatvantage be squeezed in? Converting will lose the auto updating of the menu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@draganescu Is there a "tone of voice" document somewhere for guidance on writing these sorts of user facing messages? Surely Core has something although I came up with nothing when searching.

Copy link

@mrfoxtalbot mrfoxtalbot Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, @draganescu!

  1. I had not thought about it but I get that the "we" can sound a bit odd. How about the following:
    "This is a default list of your published pages so you can get started quickly."

  2. I agree that we should mention how this change will prevent "auto-updating" from working. Maybe we could add it at the very end:
    "Please note that if you add new pages to your site, you'll need to manually add them to your navigation menu."

@jasmussen
Copy link
Contributor

jasmussen commented Oct 31, 2022

Thank you for the ping Dave, thank you for the PR Artemio and the original issue Alvaro!

It seems fine to edit the text to provide clarity. However a few notes:

  • A link to documentation is perhaps acceptable, but this feels like a "one-off" location. If I want to read about menu management, I shouldn't have to click "Edit" first, and it might not be a great precedent to provide such links.
  • That isn't to say we can't add a link to documentation. But since it links to just the navigation block page, any context provided by the modal is immediately lost and you're back to reading from the top. Unless we can link with more precision, it seems better to find a more systematic approach to adding block documentation.
  • The more text in the modal, the less likely it is someone will read it, and the more scary it looks. Anything we can do to rephase it shorter, the more clarity this can add.

The main improvement we can add is clarity around where these pages are coming from. But the current first paragraph is a little confusing to me. Perhaps something along these lines?

This navigation is showing all pages on your site. If you'd like to add, remove, or reorder items, it must be converted to a custom menu.

The following phrase was removed. Are we sure we don't need it? The fact that the Page List menu stays in sync when you add or remove pages from your site feels to me like the single most important thing to highlight.

Note: if you add new pages to your site, you'll need to add them to your navigation menu.

The following is confusing to me:

If you previously created a menu in a classic theme, converting will allow you to import it.

  • Don't classic menus automatically get converted?
  • If they don't, can't we handle this automatically, without highlighting in a message like this?

CC: @SaxonF as he's had some thoughts around repurposing the Page List block to be called "Auto-menu", that term could potentially be useful in the prose here.

@artemiomorales
Copy link
Contributor Author

Thank you all for the feedback and discussion!

I can update the pull request after @SaxonF has a chance to offer insight, and once there's agreement on what the wording should be and how to handle the link to the documentation.

@mrfoxtalbot
Copy link

Thank you for your thoughts @jasmussen

The fact that the Page List menu stays in sync when you add or remove pages from your site feels to me like the single most important thing to highlight.

I agree. We should keep this sentence. I also agree about the fact that having more text reduces the chances of people reading through and make it more scary.

But since it links to just the navigation block page, any context provided by the modal is immediately lost and you're back to reading from the top.

We could link to https://wordpress.org/support/article/navigation-block/#add-all-pages instead? (please note that the article is pending an update)

Unless we can link with more precision, it seems better to find a more systematic approach to adding block documentation.

I would love to find a systematic approach to adding unobtrusive inline support links. Are you aware of any initiatives around this?

@jasmussen
Copy link
Contributor

We could link to https://wordpress.org/support/article/navigation-block/#add-all-pages instead? (please note that the article is pending an update)

The problem is, the "Add all pages" button doesn't exist anymore, and big efforts are underway to improve the flow in #42257.

I would love to find a systematic approach to adding unobtrusive inline support links. Are you aware of any initiatives around this?

I've seen in a few places links added to the block description, here:

Screenshot 2022-11-01 at 09 29 13

However this is mainly for the most complex of blocks like query as shown here, and would not be appropriate for Paragraph, for instance.

Screenshot 2022-11-01 at 09 28 39

I recognize the navigation block has a great deal of complexity and would meet that threshold. The main challenge there is that adding more stuff to the inspector at this point only makes things worse. However there's an effort underway that might fit additional help text and/or links in the inspector: #40204

Perhaps the best part at this point is to omit the link for now, update the documentation to be fresher, and then revisit adding a link to documentation separately, perhaps after 40204 lands?

@SaxonF
Copy link
Contributor

SaxonF commented Nov 2, 2022

Perhaps the best part at this point is to omit the link for now, update the documentation to be fresher, and then revisit adding a link to documentation separately, perhaps after 40204 lands?

+1

If they don't, can't we handle this automatically, without highlighting in a message like this?

+1

A suggestion on copy below but feel free to ignore. "Convert" felt a little too techie to me as the primary CTA.

image

@artemiomorales artemiomorales force-pushed the update/revise-page-list-edit-message branch from 4c2fe7f to 585e356 Compare November 4, 2022 21:50
@artemiomorales
Copy link
Contributor Author

I updated the text with @SaxonF's suggestion.

Also, there is indeed a WordPress Brand Writing Style Guide! In particular, it recommends the use of American English, so I changed the spelling from 'Organise' to 'Organize.' (@getdave mentioned interest in a style guide like this).

Let me know how things look! Am happy to keep iterating.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for being so receptive to feedback and for continuing to iterate here.

I tested this and the wording seems fine to me with the exception of the "tip" (see comment).

) }
</p>
<p>
{ __(
"Note: if you add new pages to your site, you'll need to add them to your navigation menu."
'Tip: If you want to return to a managed menu, simply add a new menu item and pick the Pages List block.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As things stand there doesn't appear to be a way to achieve this. Try it out and you'll see what I mean.

Therefore I don't think we can include this "tip" until we have a new PR which enables this flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this is removed we're good to merge this.

@artemiomorales
Copy link
Contributor Author

@getdave is right, thanks for testing this.

When I try to add a new Page List, the original styling is lost:

page-list-bug

Even if I try to insert a new Header pattern and start over, I'm unable to recreate the header as it was originally; it utilizes a Navigation block instead of a Page List block:

header-page-list

In other words, as it stands, once a Page List block is converted inside of the default header, one is unable to return to the original implementation.

Perhaps the text should be revised to the following instead, at least until this can be improved? Or perhaps this is a bug that needs to be addressed, and we should refrain from moving forward on this issue until that is resolved.

Customize this menu

This menu is automatically kept in sync with pages on your site. You can manage the menu yourself by clicking customize below.

Warning: If you want to return to a managed menu, you can add a new Pages List block to this header, but the original styling will be lost.

@artemiomorales
Copy link
Contributor Author

After additional testing, I realized you can get the original implementation back if you add a new Navigation block to the header:

Screen Shot 2022-11-09 at 12 04 07 PM

Perhaps the text could be the following then:

Customize this menu

This menu is automatically kept in sync with pages on your site. You can manage the menu yourself by clicking customize below.

Tip: If you want to return to a managed menu, simply add a new Navigation block to this header.

@getdave
Copy link
Contributor

getdave commented Nov 9, 2022

Tip: If you want to return to a managed menu, simply add a new Navigation block to this header.

I would just remove the Tip for now. We can't hard code in a reference to a particular template (e.g. this header).

We can raise an Issue to track the need to be able to revert to a "managed menu" although I suspect it will be a less common use case.

@draganescu
Copy link
Contributor

I agree with @getdave - because the best solution to the problem the tip found is that we should have a one button action somewhere to revert to "managed menu". In other words maybe an attribute of the navigation block that overrides how the fallback performs in the sens that if attribute is true then it always shows and renders a page list block?

@SaxonF
Copy link
Contributor

SaxonF commented Nov 10, 2022

@getdave I was originally thinking about the pages block as basically a setting at the nav level of the sub nav level (e.g. just a toggle) but the issue I ran into was how to handle search, logo etc. There needs to be a "line" between what's managed and what's not, and the pages block is that line at the moment.

One potential solution is for the navigation block to focus purely on nav related items. Logo, search etc would just exist at the header level outside the navigation block.

@getdave
Copy link
Contributor

getdave commented Nov 15, 2022

@artemiomorales Is there anything I can do to help you with landing this one?

@artemiomorales artemiomorales force-pushed the update/revise-page-list-edit-message branch from 585e356 to fe07bac Compare November 16, 2022 13:04
@artemiomorales
Copy link
Contributor Author

@getdave I don't believe so! I just updated the branch and removed the tip. Just let me know if I should make any additional changes 🙏

@getdave getdave enabled auto-merge (squash) November 16, 2022 21:28
@getdave getdave merged commit 516d05c into WordPress:trunk Nov 22, 2022
@github-actions github-actions bot added this to the Gutenberg 14.7 milestone Nov 22, 2022
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a clearer explanation about how "Convert to links" works & include a link to the documentation.
8 participants