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

Blocks: Add blocks "slash" autocomplete on default block #2630

Merged
merged 15 commits into from Sep 5, 2017

Conversation

Projects
None yet
6 participants
@aduth
Member

aduth commented Aug 31, 2017

Related: #834
Supersedes: #2284

This pull request seeks to enable a user to add a block by typing a slash / in a new default paragraph block, followed by a search parameter. Using arrow keys and click/enter on a block result will replace the paragraph block with a new block of the selected type.

autocomplete

Implementation notes:

There's a bit of reinventing the wheel here, but unfortunately I wasn't able to reuse very much:

  • I wanted to use Downshift to manage the typeahead behavior, but it does not support contenteditable, only input
  • I wanted to use a utility for retrieving caret position, but textarea-caret-position applies only to textarea and input, not contenteditable, and Caret.js is dependent on jQuery
  • I wanted to use DropdownMenu for the arrow navigation of options, but I needed only menu behaviors, not the toggling button
  • I wanted to use Popover to manage positioning, but the position of the autocomplete menu cannot be determined relative a specific node (i.e. retrieving position of text caret requires custom logic)

There may still be opportunity for refactoring to consolidate some of these behaviors.

Testing instructions:

Verify that you can insert a new block from an empty paragraph block by typing slash, followed by a block search.

  1. Navigate to Gutenberg > New Post
  2. Click "Write your story"
  3. Type /
  4. Note that the autocomplete shows
  5. Type a block search
  6. Note that the autocomplete filters
  7. Use arrow keys to navigate results
  8. Click or press enter to select block
  9. Note that the empty paragraph block is replaced with a new block of the selected type
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 31, 2017

Codecov Report

Merging #2630 into master will increase coverage by 0.92%.
The diff coverage is 81.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2630      +/-   ##
==========================================
+ Coverage   31.31%   32.23%   +0.92%     
==========================================
  Files         178      181       +3     
  Lines        5429     5525      +96     
  Branches      949      968      +19     
==========================================
+ Hits         1700     1781      +81     
- Misses       3154     3166      +12     
- Partials      575      578       +3
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 33.33% <0%> (ø) ⬆️
editor/actions.js 41.17% <0%> (ø) ⬆️
blocks/editable/index.js 10.31% <0%> (-0.21%) ⬇️
blocks/block-autocomplete/index.js 100% <100%> (ø)
blocks/editable/constants.js 100% <100%> (ø)
components/popover/index.js 92.95% <77.77%> (-2.36%) ⬇️
components/autocomplete/index.js 89.04% <89.04%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da364e8...c88352a. Read the comment docs.

codecov bot commented Aug 31, 2017

Codecov Report

Merging #2630 into master will increase coverage by 0.92%.
The diff coverage is 81.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2630      +/-   ##
==========================================
+ Coverage   31.31%   32.23%   +0.92%     
==========================================
  Files         178      181       +3     
  Lines        5429     5525      +96     
  Branches      949      968      +19     
==========================================
+ Hits         1700     1781      +81     
- Misses       3154     3166      +12     
- Partials      575      578       +3
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 33.33% <0%> (ø) ⬆️
editor/actions.js 41.17% <0%> (ø) ⬆️
blocks/editable/index.js 10.31% <0%> (-0.21%) ⬇️
blocks/block-autocomplete/index.js 100% <100%> (ø)
blocks/editable/constants.js 100% <100%> (ø)
components/popover/index.js 92.95% <77.77%> (-2.36%) ⬇️
components/autocomplete/index.js 89.04% <89.04%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da364e8...c88352a. Read the comment docs.

@nicolinuxfr

This comment has been minimized.

Show comment
Hide comment
@nicolinuxfr

nicolinuxfr Aug 31, 2017

From a user point of view, I really like the idea. I think it could be a great way to add blocks. 👍

nicolinuxfr commented Aug 31, 2017

From a user point of view, I really like the idea. I think it could be a great way to add blocks. 👍

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 1, 2017

Member

Ideally, I think this should show the new block as selected after making a replacement. In fact, it is selected, but the isTyping state doesn't reset, so focus borders are hidden until the user moves their mouse over the block again. This is made difficult because isTyping only tracks a simple boolean true / false state and a REPLACE_BLOCKS action can't be tracked back to occurring within the autocomplete typeahead.

Member

aduth commented Sep 1, 2017

Ideally, I think this should show the new block as selected after making a replacement. In fact, it is selected, but the isTyping state doesn't reset, so focus borders are hidden until the user moves their mouse over the block again. This is made difficult because isTyping only tracks a simple boolean true / false state and a REPLACE_BLOCKS action can't be tracked back to occurring within the autocomplete typeahead.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 2, 2017

Contributor

In fact, it is selected, but the isTyping state doesn't reset, so focus borders are hidden until the user moves their mouse over the block again.

I think this is fine, it respects the flow of writing.

Contributor

mtias commented Sep 2, 2017

In fact, it is selected, but the isTyping state doesn't reset, so focus borders are hidden until the user moves their mouse over the block again.

I think this is fine, it respects the flow of writing.

@youknowriad

Really good job, works great.

I also see a lot of refactor opportunities:

  • I think we can use the Popover component and avoid following the cursor position (That's how slack work)

  • the dropdown component should probably be splitted, and reused accross different places (FormtTokenField, Autocomplete, Table Block, Switcher)

Show outdated Hide outdated components/autocomplete/index.js
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2017

Contributor

Made some updates to reuse the popover component. (The autocomplete is now aware of the window boundaries)

I'm postponing the accessibility changes, because this component is a bit "special". my question here is: Should we add all these combobox attributes knowing that this component won't behave as an autocomplete component if we do not type "/". It's just the paragraph block. I was thinking adding these attributes could be confusing. @afercia

Anyway, I think we can merge this and address the remaining reusability and accessibility issues separately.

Contributor

youknowriad commented Sep 4, 2017

Made some updates to reuse the popover component. (The autocomplete is now aware of the window boundaries)

I'm postponing the accessibility changes, because this component is a bit "special". my question here is: Should we add all these combobox attributes knowing that this component won't behave as an autocomplete component if we do not type "/". It's just the paragraph block. I was thinking adding these attributes could be confusing. @afercia

Anyway, I think we can merge this and address the remaining reusability and accessibility issues separately.

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Sep 4, 2017

Member

First, really enjoying using this! Great work everyone on this.

I did find one thing that maybe is more of a 'tweak' than a bug. If you do the following:

  • New Gutenberg post.
  • Type in /head to get the slash inserter.
  • Select heading.
  • Type in /heading.. you just get the words /heading

I can see a possible user flow of changing blocks through the slash command. Is this expected or a bug?

2017-09-04 at 19 05

Member

karmatosed commented Sep 4, 2017

First, really enjoying using this! Great work everyone on this.

I did find one thing that maybe is more of a 'tweak' than a bug. If you do the following:

  • New Gutenberg post.
  • Type in /head to get the slash inserter.
  • Select heading.
  • Type in /heading.. you just get the words /heading

I can see a possible user flow of changing blocks through the slash command. Is this expected or a bug?

2017-09-04 at 19 05

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2017

Contributor

@karmatosed this is expected as, this is only implemented for the paragraph block (the default block). Extending to other blocks is possible (opt-in).

Contributor

youknowriad commented Sep 4, 2017

@karmatosed this is expected as, this is only implemented for the paragraph block (the default block). Extending to other blocks is possible (opt-in).

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Sep 4, 2017

Member

Ah crumbs, totally missed it was just for default block! That totally explains it. Whilst this works for a v1, I think that adding into all blocks pretty soon after is important. I won't be only one getting hit by an expectation failure.

Member

karmatosed commented Sep 4, 2017

Ah crumbs, totally missed it was just for default block! That totally explains it. Whilst this works for a v1, I think that adding into all blocks pretty soon after is important. I won't be only one getting hit by an expectation failure.

@youknowriad youknowriad merged commit 5c8f242 into master Sep 5, 2017

3 checks passed

codecov/project 32.23% (+0.92%) compared to da364e8
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/default-block-inserter branch Sep 5, 2017

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 5, 2017

Contributor

@youknowriad

Should we add all these combobox attributes

Just tested and well, I guess you know the answer 🙂
When typing / and start typing, the block interaction changes drastically and nothing of what happens there get exposed to assistive technologies.
Actually, the block changes in a search field with suggestions, exactly like the tags suggestions and should work the same even from an a11y perspective.
It is, actually, a combobox with suggestions and should use all the ARIA already implemented for such interaction.

Contributor

afercia commented Sep 5, 2017

@youknowriad

Should we add all these combobox attributes

Just tested and well, I guess you know the answer 🙂
When typing / and start typing, the block interaction changes drastically and nothing of what happens there get exposed to assistive technologies.
Actually, the block changes in a search field with suggestions, exactly like the tags suggestions and should work the same even from an a11y perspective.
It is, actually, a combobox with suggestions and should use all the ARIA already implemented for such interaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment