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

Table block: Fix focus loss in between row/column insertions. #23508

Merged
merged 4 commits into from
Jul 6, 2020

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Jun 26, 2020

Description

Fixes #23103

This PR just allows to be able to add more rows/columns, by enabling the corresponding buttons, after having added a row/column.

To reproduce what happens now:

  • Go to a post/page
  • Add a Table block ( ex 2x2 ) and insert some text in a cell
  • Go to the Toolbar and click Add a row Before/After
  • Go to the Toolbar again and in the Dropdown menu for adding rows you'll see that everything is disabled

Other enhancements on Table block can be handled separately, such as adding keyboard support for some actions.

It needs to be revised when programmatically focus to RichText is handled, as mentioned here #9740.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@ntsekouras ntsekouras force-pushed the fix/table-enable-buttons-add-more-than-one branch from 6284ece to 71be636 Compare June 26, 2020 10:21
@ntsekouras ntsekouras self-assigned this Jun 26, 2020
@ntsekouras ntsekouras added [Block] Table Affects the Table Block [Type] Bug An existing feature does not function as intended labels Jun 26, 2020
@github-actions
Copy link

github-actions bot commented Jun 26, 2020

Size Change: +2.02 kB (0%)

Total Size: 1.13 MB

Filename Size Change
build/a11y/index.js 1.14 kB +1 B
build/annotations/index.js 3.68 kB +53 B (1%)
build/autop/index.js 2.82 kB -1 B
build/block-directory/index.js 7.67 kB +286 B (3%)
build/block-directory/style-rtl.css 944 B +3 B (0%)
build/block-directory/style.css 945 B +3 B (0%)
build/block-editor/index.js 110 kB +1.24 kB (1%)
build/block-editor/style-rtl.css 10.7 kB -10 B (0%)
build/block-editor/style.css 10.7 kB -13 B (0%)
build/block-library/editor-rtl.css 7.57 kB -23 B (0%)
build/block-library/editor.css 7.57 kB -27 B (0%)
build/block-library/index.js 130 kB +424 B (0%)
build/block-library/style-rtl.css 7.78 kB -252 B (3%)
build/block-library/style.css 7.79 kB -256 B (3%)
build/block-library/theme-rtl.css 728 B -2 B (0%)
build/block-library/theme.css 729 B -3 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB -1 B
build/block-serialization-spec-parser/index.js 3.1 kB -5 B (0%)
build/blocks/index.js 48.2 kB +77 B (0%)
build/components/index.js 199 kB +306 B (0%)
build/components/style-rtl.css 15.8 kB -74 B (0%)
build/components/style.css 15.8 kB -74 B (0%)
build/compose/index.js 9.56 kB -87 B (0%)
build/core-data/index.js 11.4 kB -1 B
build/data-controls/index.js 1.29 kB +3 B (0%)
build/data/index.js 8.44 kB +6 B (0%)
build/date/index.js 5.38 kB -89 B (1%)
build/edit-navigation/index.js 9.92 kB +55 B (0%)
build/edit-navigation/style-rtl.css 1.02 kB -1 B
build/edit-navigation/style.css 1.02 kB -1 B
build/edit-post/index.js 304 kB +215 B (0%)
build/edit-post/style-rtl.css 5.57 kB +67 B (1%)
build/edit-post/style.css 5.57 kB +66 B (1%)
build/edit-site/index.js 16.7 kB +18 B (0%)
build/edit-site/style-rtl.css 3.03 kB -7 B (0%)
build/edit-site/style.css 3.03 kB -8 B (0%)
build/edit-widgets/index.js 9.35 kB +28 B (0%)
build/edit-widgets/style-rtl.css 2.45 kB +26 B (1%)
build/edit-widgets/style.css 2.45 kB +26 B (1%)
build/editor/index.js 45 kB +252 B (0%)
build/editor/style-rtl.css 3.77 kB -72 B (1%)
build/editor/style.css 3.77 kB -78 B (2%)
build/element/index.js 4.65 kB +1 B
build/format-library/index.js 7.73 kB +3 B (0%)
build/hooks/index.js 2.13 kB +2 B (0%)
build/i18n/index.js 3.56 kB -1 B
build/is-shallow-equal/index.js 710 B -1 B
build/keyboard-shortcuts/index.js 2.52 kB +5 B (0%)
build/keycodes/index.js 1.94 kB +3 B (0%)
build/list-reusable-blocks/index.js 3.12 kB -1 B
build/list-reusable-blocks/style-rtl.css 476 B +26 B (5%) 🔍
build/list-reusable-blocks/style.css 476 B +25 B (5%) 🔍
build/media-utils/index.js 5.32 kB +27 B (0%)
build/notices/index.js 1.79 kB +2 B (0%)
build/nux/index.js 3.41 kB +3 B (0%)
build/nux/style-rtl.css 671 B +8 B (1%)
build/nux/style.css 668 B +8 B (1%)
build/plugins/index.js 2.56 kB -2 B (0%)
build/primitives/index.js 1.41 kB -92 B (6%)
build/rich-text/index.js 13.9 kB -94 B (0%)
build/server-side-render/index.js 2.71 kB +36 B (1%)
build/shortcode/index.js 1.69 kB -1 B
build/url/index.js 4.06 kB +1 B
build/viewport/index.js 1.85 kB -1 B
build/wordcount/index.js 1.17 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/api-fetch/index.js 3.4 kB 0 B
build/blob/index.js 620 B 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/html-entities/index.js 622 B 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/warning/index.js 1.14 kB 0 B

compressed-size-action

This just allows to be able to add more rows, by enabling the corresponding buttons.

It needs to be revised when programmatically focus to RichText is handled.
@ntsekouras ntsekouras force-pushed the fix/table-enable-buttons-add-more-than-one branch from 71be636 to 22daaf8 Compare June 29, 2020 06:23
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

I think some clear testing instructions for this pull request would be very helpful, since the issue isn't obvious to reproduce consistently.

I'm glad you link to #9740, I think that's useful context to have. That said, my first impression is that we can best address the current Table issue by finding what — if anything — we can do at the DOM-event level to mitigate the issue.

Comment on lines 302 to 308
// this just allows to be able to add more rows, by enabling the corresponding
// buttons. It needs to be revised when programmatically focus to RichText is handled
this.createOnFocus( {
sectionName,
rowIndex: newRowIndex,
columnIndex: 0,
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

By definition, createOnFocus just returns a new function but doesn't do anything with it. So I'm fairly sure that nothing is happening here. :)

Copy link
Contributor Author

@ntsekouras ntsekouras Jul 6, 2020

Choose a reason for hiding this comment

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

Thanks Miguel. I saw the code now and couldn't understand why I left it that way... Maybe when I was trying to find how to focus on RichText..?

I changed it to just change the selected cell in state.

Comment on lines 363 to 368
// this just allows to be able to add more rows, by enabling the corresponding
// buttons. It needs to be revised when programmatically focus to RichText is handled
this.createOnFocus( {
rowIndex: 0,
columnIndex: newColumnIndex,
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

Same observation as for rows.

packages/block-library/src/table/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/table/edit.js Outdated Show resolved Hide resolved
ntsekouras and others added 2 commits July 6, 2020 18:50
Co-authored-by: Miguel Fonseca <[email protected]>
Co-authored-by: Miguel Fonseca <[email protected]>
@ntsekouras ntsekouras merged commit e49c03b into master Jul 6, 2020
@ntsekouras ntsekouras deleted the fix/table-enable-buttons-add-more-than-one branch July 6, 2020 16:41
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 6, 2020
@mcsf
Copy link
Contributor

mcsf commented Jul 6, 2020

Nice fix!

@cngodles
Copy link

cngodles commented Jul 7, 2020

Thank you. I'll be so glad when I can add rows easier. I have a few situations where I need to go to an existing table and add 20-30 at a time. This will cut down on clicks.

youknowriad pushed a commit that referenced this pull request Jul 13, 2020
* Enable add row/column buttons after having added one.

This just allows to be able to add more rows, by enabling the corresponding buttons.

It needs to be revised when programmatically focus to RichText is handled.

* set selected cell in state

* Change comment description

Co-authored-by: Miguel Fonseca <[email protected]>

* Change comment description

Co-authored-by: Miguel Fonseca <[email protected]>

Co-authored-by: Miguel Fonseca <[email protected]>
@mcsf mcsf changed the title Enable add row/column buttons after having added one Table block: Fix focus loss in between row/column insertions. Jul 21, 2020
@mcsf mcsf added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Table Affects the Table Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table Block - Adding More than One or Multiple Rows
3 participants