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

Nav offcanvas remove "Browse All" from inserter block picker #47014

Closed
getdave opened this issue Jan 10, 2023 · 8 comments
Closed

Nav offcanvas remove "Browse All" from inserter block picker #47014

getdave opened this issue Jan 10, 2023 · 8 comments
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective.

Comments

@getdave
Copy link
Contributor

getdave commented Jan 10, 2023

As reported in #46939 (comment), when picking blocks from the Navigation offcanvas using the inserter there is a Browse All button. However, when clicked this breaks focus away from the offcanvas and should be removed.

@getdave getdave added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Block] Navigation Affects the Navigation Block [a11y] Keyboard & Focus labels Jan 10, 2023
@draganescu
Copy link
Contributor

Three things I noticed trying to work on this issue:

  1. The browse all button only seems to show up if the navigation block is inserted in the post editor.
  2. What about the blocks that don't fit? Rely only on search?
  3. In case we opt to leave it the way things happen is:
  • insert nav block
  • go to right hand list view
  • click on browse all
  • go to left hand inserter
  • insert block
  • left hand inserter closes
  • focus is on the newly inserted block
    Is this OK?

@getdave getdave added the Needs Design Feedback Needs general design feedback. label Jan 10, 2023
@draganescu draganescu self-assigned this Jan 10, 2023
@draganescu draganescu added the Needs Technical Feedback Needs testing from a developer perspective. label Jan 10, 2023
@draganescu
Copy link
Contributor

I seems an ancient #23737 introduced a check for setInserterIsOpened a method that comes from settings and somehow it is unavailable in the site editor. So far nobody complained about the browse all missing in the site editor but it is missing ever since I think? Why? Because removing the check for setInserterIsOpened makes the browse all buttons show up.

Not sure how to proceed :)

@getdave
Copy link
Contributor Author

getdave commented Jan 10, 2023

@ntsekouras Has been working on inserter a lot recently I think. Maybe he would know...?

@ntsekouras
Copy link
Contributor

Hm.. It seems it's been removed here. @youknowriad do you remember if there was there a specific reason for it?

@youknowriad
Copy link
Contributor

Sorry, not sure I understand what was removed in my PR?

@youknowriad
Copy link
Contributor

Ok I see what you mean, it was an inadvertent change, I'll fix it.

@draganescu
Copy link
Contributor

I don't think we should remove the browse all button. Instead we should figure out something that tackles the context switching better. The problem in @alexstine 's review was that once the block was selected from the inserter the focus was not sent back to the list view in the inspector, but it was sent to the canvas instead.

Removing the browse all button hides blocks that can be inserted but don't fit in the six slots the quick inserter uses. There is search but one has to know what to search for.

@getdave
Copy link
Contributor Author

getdave commented Jan 12, 2023

The problem in @alexstine 's review was that once the block was selected from the inserter the focus was not sent back to the list view in the inspector, but it was sent to the canvas instead.

See #47018

Removing the browse all button hides blocks that can be inserted but don't fit in the six slots the quick inserter uses. There is search but one has to know what to search for.

Ok let's close this one with the context that we didn't tackle it because there's a necessity to have the ability to browser all the blocks.

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 [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective.
Projects
None yet
Development

No branches or pull requests

5 participants