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

Move navigation error state to the inspector #44486

Open
draganescu opened this issue Sep 27, 2022 · 31 comments · May be fixed by #53930
Open

Move navigation error state to the inspector #44486

draganescu opened this issue Sep 27, 2022 · 31 comments · May be fixed by #53930
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Block] Page List Affects the Page List Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Bug An existing feature does not function as intended

Comments

@draganescu
Copy link
Contributor

draganescu commented Sep 27, 2022

Description

Originally found by @jordesign this is a Gutenberg UX bug not a theme implementation issue. There is no clear programmatic fix, I think we need a better state for this edge case:

Step-by-step reproduction instructions

  • user has no pages
  • user has no menus
  • user switches to a block theme
  • user sees the error message: "Page List: Cannot Retrieve Pages." in the editor
  • nothing is rendered on the front end

Screenshots, screen recording, code snippet

Editor

Screenshot 2022-09-27 at 09 44 15

Rendered site

Screenshot 2022-09-27 at 09 44 45

Proposed solution

Image

Situation: the menu that’s ref’ed is deleted.

Result: Page List is shown in the canvas, an error message is shown in the inspector, with two buttons:

“Keep pages”, it keeps the page list, removes the ref, marks the document dirty.
“Create menu”, creates a new menu, updates the ref, marks the document dirty.
@draganescu draganescu added [Type] Bug An existing feature does not function as intended Needs Design Feedback Needs general design feedback. [Block] Navigation Affects the Navigation Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Block] Page List Affects the Page List Block labels Sep 27, 2022
@draganescu
Copy link
Contributor Author

@javierarce @jasmussen would you say defaulting to one "home link" is better than this situation?

@javierarce
Copy link
Contributor

@draganescu I'd say yes, it's better to show something users can change than that error message, which doesn't help you to solve the issue.

@jordesign
Copy link
Contributor

I'd say yes as well. I'd also be really keen to see the editor view match the front-end view as much as possible, especially in the default state.

@jasmussen
Copy link
Contributor

Ah, this is an interesting one, because an empty page list is accurate when there are no pages, whereas a "home link" would not be, and it might not even be user intent. Given a new WordPress install defaults to adding "About", that seems to further reinforce that if these pages are intentionally deleted, the user probably wants an empty menu. So I would avoid adding any home links or otherwise.

In fact, as a fix to this, I'd show nothing! This would be accurate to the frontend, and probably to user intent. It seems like it should be an easy fix, we just want to make sure the block doesn't collapse into nothingness and can't be selected — so it probably needs a min-width and min-height so it's at least selectable.

A harder fix I'd suggest to be low priority: show a literal placeholder, same as what you'd see if you have no site logo and insert one. (In 6.1 that shows a dashed outline indicating: there's a placeholder here that can be filled out with something, and for 6.2 a new style is being explored, see #44460)

@mrfoxtalbot
Copy link

I would favor showing a clearer and actionable placeholder text, something like this?

You have no pages yet. Add one? +

Screen Shot on 2022-09-30 at 15:54:10

I would also ensure that the "+" add icon is always visible. Would it be possible to default to selecting the Navigation Block instead of the Page List Block if there are no pages?

@jasmussen
Copy link
Contributor

What if you don't want any pages, though? What if the context is so constrained there isn't space to add a message?

@mrfoxtalbot
Copy link

Mmmm... I see your point, @jasmussen. A generic placeholder would also work, but I feel that some text would be more inteligible.

Regarding the "user doesn't want any pages" scenario, they could ignore it or even better, remove the navigation block completely?

@noisysocks noisysocks added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Nov 4, 2022
@artemiomorales artemiomorales self-assigned this Nov 17, 2022
@priethor priethor added this to Polish Apr 28, 2023
@artemiomorales artemiomorales removed their assignment Apr 28, 2023
@priethor priethor moved this to 💻 Needs development in Polish Apr 28, 2023
@mklute101
Copy link
Contributor

Created a PR with @mrfoxtalbot suggestion.

@jasmussen
Copy link
Contributor

I approved the PR, because it improves the situation. But I don't think it closes this conversation. My original opinion stands:

  • The canvas itself being WYSIWYG is not the right place to show error messages. I don't even know how screen-reader users would know to look there and differentiate content from warnings.
  • Layouts in the canvas may be able to accommodate a collapsed hamburger menu for a navigation, but not an error message. Therefore if you activate a theme that does this, but have no pages, your layout will be broken in the editor, even if it's correct on the frontend.

An argument was shared that if you don't have any pages, you can delete the menu. That's true, but the onus to "fix a layout" should not be on the user who just installed a block theme.

To that end, I maintain that a page list block should show nothing at all when there are no pages, that this is the intended behavior, that it seems a simple fix. CC: @WordPress/gutenberg-design

@jameskoster
Copy link
Contributor

To that end, I maintain that a page list block should show nothing at all when there are no pages, that this is the intended behavior, that it seems a simple fix.

I'd agree with that, especially when the block is not selected. Perhaps a message could appear upon selection to clarify why the block is empty (and offer a way to draft a new page), but that would need some design exploration.

@mklute101
Copy link
Contributor

Would it make sense for the language to be the same as an empty post list/query loop? "No results found"

I posted this in the PR. First time contributing so unsure where the discussion should be centered, but posting here as well for visibility.

@draganescu
Copy link
Contributor Author

draganescu commented Aug 28, 2023

@jameskoster if the block does not display anything if there are no pages how do you know it's there to select it?

@jasmussen
Copy link
Contributor

Same as with a spacer. The bigger question is, does it need to be selected? Pages will show up as soon as you publish some.

@draganescu
Copy link
Contributor Author

Spacer could be argued to be more intuitive as clicking on empty space should select the space. But inserting a nav block and then getting an empty space is not the same.

@jasmussen
Copy link
Contributor

They are separate blocks, sure, and I think it's valid enough to show a message in the inspector about the navigation being empty when you do select it. But it isn't clear to me why this is a case of delineating WYSIWYG. Similar to classic themes that define a location for a menu to be inserted, and showing nothing until you create one, why does it have to be different here? While you should be able to insert a navigation block anywhere, especially when you build out a theme, for the majority of people it seems likely to think their navigation block will be preinserted in a header, by the theme they activate. And rather than show them an error message that suggests something is in need of correcting, just let it be invisible until a blogger chooses to create an "About" page.

@draganescu
Copy link
Contributor Author

just let it be invisible

Doesn't this sound weird? There is a content block that is invisible, but can randomly be found in the tree of blocks (list view). It looks broken, when in fact it's behaving correctly. I think this is yes a case where WYSIWYG should remember editing is not consuming - show a placeholder or something for content that has something going on.

@jasmussen
Copy link
Contributor

I hear where you're coming from, but I also think that's misstating what's happening. It's a content block that has nothing to show, and so it shouldn't show anything that isn't there, or isn't meant to be there. It is arguably a templating block that is meant to be there to provide a feature for a theme. And if the user of that theme doesn't want pages on their site, it shouldn't warn them every time they enter the site editor, or in the future the post editor with templates showing. It should be there so that suddenly pages appear in a considered place once the user decides to publish some.

@draganescu
Copy link
Contributor Author

I also think that's misstating what's happening.

Why then does the group block show a border or the image show a placeholder? Why is the navigation block special? Do we just want to basically discourage people from building menus? 😁

@jasmussen
Copy link
Contributor

We definitely don't want to discourage people from building menus, that's why by default, especially if you leave the navigation block in place rather than deleting it because of a confusing error message, it will automatically populate when you publish pages. And then you can select it if you need to customize it further.

The group block and image blocks only show a placeholder when they are entirely empty. And even those cases, the patterns they follow are outdated and need to be updated. The site logo block, for example, is a good example of where I could see that generic placeholder pattern go, lean much more into the actual literal placeholder pattern you'd see from desktop publishing patterns of the old days. The main reason why we need to evolve beyond Placeholder component is exactly the same reason why it's a bad idea for the navigation block: it does not respect the surrounding layout. If you insert a Group block into a narrow column, it'll wrap awkardly and be illegible. The canvas, in other words, is just not a good place to show error messages.

Also, both those examples diverge from the navigation block in that you almost certainly wouldn't insert empty image or group blocks in a header or footer template, whereas when building a theme, you definitely would.

Just to broaden the conversation here, I'd really like to ping @WordPress/gutenberg-design yet again. On my side, it's not the strongest opinion in the world that the navigation block is truly invisible when no pages are published, the only strong opinion I have is that whatever message we do show does not cause layout shifts.

@jameskoster
Copy link
Contributor

Also, both those examples diverge from the navigation block in that you almost certainly wouldn't insert empty image or group blocks in a header or footer template, whereas when building a theme, you definitely would.

This captures my perspective, in a nutshell. There aren't really legitimate scenarios that call for empty Image blocks, hence the placeholder. On the other hand an empty nav is perfectly legitimate – a theme might include the block, and the site may not have any published pages yet.

There's definitely an argument that the user might want to consider removing the block if they don't intend to ever add any nav links – but there's no reliable way to know that which makes a persistent on-canvas notice seem heavy-handed.

@draganescu
Copy link
Contributor Author

draganescu commented Aug 29, 2023

The site logo block, for example, is a good example of where I could see that generic placeholder pattern go

This is showing a placeholder if there is no logo. The placeholder implementation may be better than say group's but it's there. This also can be added by a theme, just like say a featured image, and then should it not show if there is no logo?

I think both of you have a good point, except that it's IMO somehow unfinished. What is the general direction we're moving into? Should all blocks that have nothing to display show nothing in the editor in specific contexts?

The navigation block is not empty when there are no pages, its displaying an empty page list block. The page list block is actually empty, the navigation block is not. It will also afaik render empty markup. I also suspect that this particular edge case will make people insert a new navigation block since they can't tell there is one already. And the same is true for all other blocks which are invisible if empty. The spacer block is the exception because it does show up as empty space.

On my side, it's not the strongest opinion in the world that the navigation block is truly invisible when no pages are published, the only strong opinion I have is that whatever message we do show does not cause layout shifts.

Yes, I agree with this. We need a better placeholder system that is unified across blocks, does not cause layout shifts, is selectable and it communicates there is something there which is likely waiting on user action: configure or remove. This is the problem to solve in my head too.

I too don't want error messages or informational messages in the canvas, simply because they're making the experience bad. We have the block inspector, toast notices, big yellow notices, lots of places. But we still have these messages because we don't have rules as to how placeholders work for when stuff is broken or unconfigured: what should we show, where to put the instructions?

It should also encompas problems with block rendering itself and avoid this ugliness:

<Block className="has-warning">
	<BlockInvalidWarning clientId={ clientId } />
	<RawHTML>{ safeHTML( saveContent ) }</RawHTML>
</Block>

@jasmussen
Copy link
Contributor

This is showing a placeholder if there is no logo. The placeholder implementation may be better than say group's but it's there. This also can be added by a theme, just like say a featured image, and then should it not show if there is no logo?

This is because the site logo block itself, at least currently, is the primary interface for adding a logo. That's why there needs to be a button. The navigation block is not the primary interface for adding pages. With the off-canvas inspector, and the new navigation section, the block in the canvas is less and less the primary interface for even adding menu items.

But we still have these messages because we don't have rules as to how placeholders work for when stuff is broken or unconfigured: what should we show, where to put the instructions?

I agree we need clearer documentation for how these patterns can and should work. Part of this is a chicken and egg situation where the downsides to the existing patterns keep showing themselves, but the bandwidth and priority to fixing existing placeholders has not been present. Additionally, blocks can inform exactly the path forward for these blocks, so that we can extract systems and patterns from what works well in practice. That to me is an argument to move forward and learn from the Navigation block here.

Agree, the invalid block rendering is atrocious. Here's an earlier mockup exploring that, which I believe is ticketed somewhere but not on anyone's radar:
invalid-blocks-1

missing-blocks

@jameskoster
Copy link
Contributor

There's also #41747 which contemplates displaying block errors in list view.

@draganescu
Copy link
Contributor Author

I like the ideas in #41747 @jameskoster. If we incorporate the last comment it could be the solution to this and then we could apply Joen's vision with just not showing anything.

If we do that then we should go through the affected blocks in the library and maybe make them aware that they're "empty" so they don't render empty markup either.

@jasmussen
Copy link
Contributor

Yep, lots of notices worth showing:

  • Missing API key in your openstreetmap block
  • Broken link
  • Page that's linked to was deleted (when we have page link tokens)
  • Menu was deleted, (perhaps that would let us just seamlessly fallback to showing "all pages" instead of the error message)
  • Missing plugin
  • Invalid content
  • Missing image

@annezazu annezazu removed this from Polish Jul 24, 2024
@jasmussen
Copy link
Contributor

This came up again recently. To try and unstick this, but without this being dependant on a linting interface, here's a new mockup:

Image

Situation: the menu that’s ref’ed is deleted.

Result: Page List is shown in the canvas, an error message is shown in the inspector, with two buttons:

  • “Keep pages”, it keeps the page list, removes the ref, marks the document dirty.
  • “Create menu”, creates a new menu, updates the ref, marks the document dirty.

@jasmussen jasmussen removed the Needs Design Feedback Needs general design feedback. label Oct 17, 2024
@jasmussen jasmussen moved this to Next in Design priorities Oct 17, 2024
@draganescu
Copy link
Contributor Author

This is perfect as a stop gap. I'll move to description the mockup you shared @jasmussen 👏🏻

@draganescu draganescu changed the title Navigation block shows unintuitive message if no menus and no pages exist Move navigation error state to the inspector Oct 17, 2024
@draganescu draganescu added the Needs Dev Ready for, and needs developer efforts label Oct 17, 2024
@jameskoster
Copy link
Contributor

+1 this looks good.

It's a tiny detail but I'm wondering about the button appearance. Is there a more common action here, and if so should we use a combination of button variants to represent that (primary + secondary, secondary + tertiary, etc)?

If we feel equal prominence makes sense then we might consider a more subtle variant (link?) applied to both, to reduce the button footprint. The notice is already drawing plenty of attention.

@getdave
Copy link
Contributor

getdave commented Dec 3, 2024

I just thought - what happens if there are no Pages on the Site (as per the original Issue description)? What do we show in canvas?

@jasmussen
Copy link
Contributor

What do we show in canvas?

Unless selected, do we need to show anything? If there are no pages on the site, then presumably the user is either building out their site, or they want a single-pager, and desire no pages to show up. Being able to have a Header template that still includes a navigation block, so that if one day the user chooses to publish some pages, they will automatically start showing up there, is still beneficial. If it continues to show an error message in the canvas, we imply the user is doing something wrong, which isn't necessarily the case.

Options here are wide, though, the navigation block can show its placeholder state if it's selected, or if any ancestor is selected. To support that, it would be good to have a very basic placeholder state, even more basic than the current one we have today. Just like how the Post Content block shows some text, so you can style it, the placeholder state could show just "Navigation (empty)", in the same font and style and size as a navigation item would be. What do you think?

@talldan
Copy link
Contributor

talldan commented Dec 18, 2024

I noticed something related to this issue. I was trying to reset some small changes to my site to its initial state, so I reset my templates, template parts, and I deleted my menus.

However, every navigation block I insert seems to incorrectly reference the deleted menu:

Kapture.2024-12-18.at.10.28.48.mp4

I also couldn't delete the ref from the code editor, it comes back again! It does fix itself if I reload the page.

I think the proposed changes here would fix it, so I haven't made a separate bug report.

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 [Block] Page List Affects the Page List Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Type] Bug An existing feature does not function as intended
Projects
Status: Next
Development

Successfully merging a pull request may close this issue.