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

Navigation: Try removing gray blobs. #36809

Closed
wants to merge 1 commit into from

Conversation

jasmussen
Copy link
Contributor

Description

When you start fresh with a navigation block and insert an empty one, you get these gray blobs:

Screenshot 2021-11-24 at 09 19 09

The initial idea was to let themes preinsert a navigation block at an appropriate place in the theme, but leave it otherwise unconfigured. With that goal in mind, the gray blobs were meant to help the placeholder state pass the "squint test", to look at least a little bit like the end result at least when the block was unselected.

Feedback suggested this didn't quite work as intended, though: the blobs looked like a "skeleton loading state", that they would be replaced with the real menu once loading had completed.

There's persistence in the navigation block now, with contents saved separately, and options for providing defaults and fallbacks. Depending on where that lands, we might be able to provide better first run experiences. In any case, it changes the equation from before, where an empty and unconfigured navigation block was more likely to appear in themes.

In light of this, it feels worth it to explore removing those blobs entirely, which this PR does:

gray blobs

As an added benefit, this simpler setup state entirely avoids a layout shift when selecting and unselecting.

Related PRs:

How has this been tested?

Insert a navigation block and observe a setup state that shows "Navigation" when unselected, and "Navigation" with some options when selected.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@jasmussen jasmussen added the [Block] Navigation Affects the Navigation Block label Nov 24, 2021
@jasmussen jasmussen self-assigned this Nov 24, 2021
@jasmussen jasmussen added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Nov 25, 2021
@jasmussen
Copy link
Contributor Author

It looks like I missed the fact that the PlaceholderPreview component is being used to indicate loading states in a few places. I think I'll try a different approach.

@jasmussen
Copy link
Contributor Author

Closing in favor of #36858.

@jasmussen jasmussen closed this Nov 25, 2021
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 [Status] Blocked Used to indicate that a current effort isn't able to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant