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

Alternative: add inserter to Nav block offcanvas experiment #45947

Merged
merged 8 commits into from Nov 23, 2022

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 21, 2022

What?

Alternative to #45905.

This version just uses the simple inserter + that looks similar to the only in the canvas.

Please note: this does not yet colocate the inserter popup with the offcanvas list view. That will be tackled in a followup.

Why?

Addresses the first part of #45445.

How?

Adds a customised block appender by utilising the <Inserter> component directly.

This implementation deliberately hardcodes all TreeGridRow props to reflect the root level positioning. In future we may wish to have the appender at every level of the list view but that isn't currently the case so I'm working towards landing the simplest possible implementation.

Testing Instructions

  1. Enable offcanvas experiment.
  2. New Post -> Add Nav block.
  3. Open offcanvas
  4. See appender.
  5. Click appender.
  6. See that it opens the inserter in the canvas (that's intentional....for now).
  7. Test that you can navigate the List View using keyboard and it's possible to access the appender using keyboard (arrows).

Screenshots or screencast

Screen Shot 2022-11-21 at 16 10 03

@getdave getdave added the [Block] Navigation Affects the Navigation Block label Nov 21, 2022
@getdave getdave self-assigned this Nov 21, 2022
@codesandbox
Copy link

codesandbox bot commented Nov 21, 2022

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

@github-actions

This comment was marked as off-topic.

Comment on lines 214 to 220
<tr>
<td>
<OffCanvasEditorAppender
rootClientId={ clientId }
/>
</td>
</tr>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any focusable element should be wrapped in <TreeGridRow> and <TreeGridCell> instead of <tr> and <td> so that keyboard navigation works.

I see ListView has AnimatedTreeGridRow, not sure if it needs to use that. I think that's for the block movement animation.

There may be a few extra requirements like increasing the setSize prop (the number of items in a branch of the tree) on the TreeGridRow by one when there's an appender.

Also TreeGridCell has callback style children, and the focusable should be able to receive the ref, tabIndex and onFocus props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL about these components auto managing focus. Thanks! Will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talldan I updated things. Does that look better?

I haven't dealt with setSize. I suppose that's when the appender is hidden/shown we're going to need to dynamically update that?

Copy link
Contributor

@talldan talldan Nov 23, 2022

Choose a reason for hiding this comment

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

TreeGridRow does take some props:

  • level: the level/depth of nesting
  • position: the row number
  • setSize: the number of rows

They correspond to the aria props (aria-level, aria-setsize, aria-posinset), more info here - https://www.w3.org/WAI/ARIA/apg/example-index/treegrid/treegrid-1.

Something that I didn't notice until now is that I think it would be easier to move the code to the TreeGridBranch component, as it already has those variables ready for you to pass as props (level, position, blockCount) and it will only take minor adjustment to include the appender as a row. Rendering after the array map in that component should have a similar outcome.

At the moment, the way it works in this PR is that the appender only shows at root level, if you want to keep that behavior you can do conditional rendering (when level === 1), but moving the implementation to TreeGridBranch would also allow you to render an appender at every depth (it would be visually very busy, so possibly something to iterate on).

May also need to double-check the keyboard navigation still works as expected as I remember last time the movers were made visible it was buggy. I can definitely help with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. I didn't know about those aria props. I'll take another look at this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, the way it works in this PR is that the appender only shows at root level, if you want to keep that behavior you can do conditional rendering (when level === 1), but moving the implementation to TreeGridBranch would also allow you to render an appender at every depth (it would be visually very busy, so possibly something to iterate on).

I tried this out. The problem was with level set to 1 the appender showed after every root level block. This isn't what we want yet so I'd prefer to avoid overcomplicating this PR with any additional logic or changes required to achieve that,

I agree it's something we may want in the future and we can look to refactor towards that when that becomes a requirement.

I appreciate all your feedback on this aspect. It's been extremely helpful 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added all the required props and hard coded them to reflect the root level.

@getdave getdave force-pushed the try/add-block-appender-to-nav-offcanvas branch from c1213cd to 2ad866f Compare November 23, 2022 09:36
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

This looks like a good first step. Ideally we'd open the appender in the sidebar, but we can do that in a followup.

@getdave
Copy link
Contributor Author

getdave commented Nov 23, 2022

This looks like a good first step. Ideally we'd open the appender in the sidebar, but we can do that in a followup.

Created a followup.

@getdave getdave merged commit 3a5f1f0 into trunk Nov 23, 2022
@getdave getdave deleted the try/add-block-appender-to-nav-offcanvas branch November 23, 2022 11:32
@github-actions github-actions bot added this to the Gutenberg 14.7 milestone Nov 23, 2022
@getdave getdave added the [Type] Experimental Experimental feature or API. label Nov 30, 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 Needs User Documentation Needs new user documentation [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants