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

Changed the message when there are no pages from an error message to … #53930

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

mklute101
Copy link
Contributor

@mklute101 mklute101 commented Aug 24, 2023

Resolves #44486

@ndiego Another first contribution 👍

What?

When a page list block is empty, rather than returning an error message, simply state that there are no pages.

Why?

The current error message is unintuitive.

How?

It changes the error message to text

Testing Instructions

  1. Set all pages in the test env to draft
  2. Edit the header or footer template part
  3. Insert a page list block.
  4. Review the message

Testing Instructions for Keyboard

Screenshots or screencast

Screenshot 2023-08-24 at 4 14 33 PM

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

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @mklute101! 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.

@priethor priethor added [Type] Bug An existing feature does not function as intended [Block] Page List Affects the Page List Block labels Aug 24, 2023
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

I'd call it "You have no pages." Also, please get a code check on this one.

As a small step forward, let's ship this. That said, I still don't know that we want to show anything at all here. But I'll share my arguments on #44486 and we can look at that as a followup. Thanks for the PR!

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 your PR 🙇

I'd like to confirm if this changed markup is perceivable by users of assistive technology. The Notice component comes with a set of built in affordances that are very important.

I sense the root of this PR is about the wording - I'm happy with that. However, I would like to understand why we decided to change the components used. What is the justification?

Many thanks

@jasmussen
Copy link
Contributor

There's a bit more conversation in #44486 that touches specifically on this, notably that I'm questioning the usability of showing notice content in the canvas at all. I will defer to experts in the field, but my intuition would be that the content canvas would not be where I'd look for error notices, were I a screen-reader user. To that end, and to ensure actual WYSIWYG, I really think we should move towards having the canvas for content, and the UI for notices and error messages.

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 don't see the new message :(

Screenshot 2023-08-25 at 12 06 23

In the screenshot the nav block is there but invisible - while on trunk the error in page list shows.

<span
className="rich-text"
data-rich-text-placeholder="You have no pages yet."
></span>
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested and just using a simple span is better, no className and no data-rich-text-placeholder. Just a span with the text passed through __.

@jameskoster
Copy link
Contributor

"You" infers site ownership upon the current user which may not be appropriate in enterprise environments with may users with different roles. Perhaps something like "No pages found" would work better?

@mklute101
Copy link
Contributor Author

Would it make sense for the language to be the same as an empty post list/query loop? "No results found"
Screenshot 2023-08-26 at 10 42 16 AM

@draganescu
Copy link
Contributor

@mklute101 it should be more specifc than "results" because this is a more specific block trying to fall back to a default and it can't - because there are no pages. So we need to talk about pages.

Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mklute101 <[email protected]>
Co-authored-by: jasmussen <[email protected]>
Co-authored-by: getdave <[email protected]>
Co-authored-by: draganescu <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Co-authored-by: javierarce <[email protected]>
Co-authored-by: jordesign <[email protected]>
Co-authored-by: mrfoxtalbot <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Page List Affects the Page List Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move navigation error state to the inspector
6 participants