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

List View: avoid re-rendering all items on block focus. Enable persistent List View in the widget editor. #35706

Merged
merged 17 commits into from
Oct 22, 2021

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Oct 16, 2021

Proposed changes in this PR move the querying of isSelected to the ListViewBlock to avoid slowing down the editor on block focus.

The key thing to highlight is block focus on this branch is 221.22 ms vs 930.92 ms on trunk with List View open. Changes here don't affect first render time which still sits in the 3-5 second range to open List View with the performance test post. Using a form of windowing should cut first render down to a more respectable 200ms see #35230

Functionality Changes

The widget editor was the only consumer of the ListView dropdown (BlockNavigationDropdown). This required some isSelected state to render a subset of items. From the scrollbars in the popover seen in trunk this isn't a well known usage, so I'm proposing that we use a ListView sidebar instead for consistency. If this change is too large, I can pull out this change to a separate PR.

before block selected before with no blocks selected After Sidebar
CleanShot 2021-10-18 at 10 50 28@2x CleanShot 2021-10-18 at 10 50 22@2x CleanShot 2021-10-18 at 15 19 52@2x

Testing Instructions

  • No regressions in list view
  • Expand/Collapse works in all list view instances
  • No drag regressions
  • Animations are still seen when using the mover controls
  • Widget editor inserter, list view and general sidebar interact well.

Full Performance Results

Edit: note that performance suite changes were removed in a rebase to avoid slowing down performance tests in trunk before landing. (What I measured with can be seen at c6b08e5)

To give us solid numbers on the proposed changes, I've also included changes in the performance tests to have list view open during focus and typing in addition to a new test that toggles the list view open and closed.

>> post-editor

┌──────────────────────┬──────────────────────────────────────────┬──────────────┐
│       (index)        │ 6d49143e3d63aed79b54ccc10ddac1918b432f79 │    trunk     │
├──────────────────────┼──────────────────────────────────────────┼──────────────┤
│    serverResponse    │                '239.1 ms'                │ '225.86 ms'  │
│      firstPaint      │                '56.3 ms'                 │  '46.98 ms'  │
│   domContentLoaded   │               '275.36 ms'                │ '276.96 ms'  │
│        loaded        │               '287.34 ms'                │  '286.4 ms'  │
│ firstContentfulPaint │               '5541.68 ms'               │ '5432.78 ms' │
│      firstBlock      │               '6302.24 ms'               │ '6226.54 ms' │
│         type         │                '59.29 ms'                │  '61.05 ms'  │
│       minType        │                '53.18 ms'                │  '53.41 ms'  │
│       maxType        │                '72.09 ms'                │  '75.38 ms'  │
│        focus         │               '185.12 ms'                │ '936.54 ms'  │
│       minFocus       │                 '1.2 ms'                 │   '1.2 ms'   │
│       maxFocus       │               '238.13 ms'                │ '1613.51 ms' │
│     inserterOpen     │                '78.58 ms'                │  '80.01 ms'  │
│   minInserterOpen    │                '54.4 ms'                 │  '53.94 ms'  │
│   maxInserterOpen    │               '258.84 ms'                │ '249.32 ms'  │
│    inserterSearch    │                '92.41 ms'                │  '87.78 ms'  │
│  minInserterSearch   │                '58.03 ms'                │  '54.26 ms'  │
│  maxInserterSearch   │               '291.23 ms'                │ '290.97 ms'  │
│    inserterHover     │                '38.41 ms'                │  '38.62 ms'  │
│   minInserterHover   │                 '35 ms'                  │  '32.74 ms'  │
│   maxInserterHover   │                '53.24 ms'                │  '45.12 ms'  │
│     listViewOpen     │               '3358.94 ms'               │ '3074.26 ms' │
│   minListViewOpen    │               '2976.39 ms'               │ '2843.87 ms' │
│   maxListViewOpen    │               '4778.34 ms'               │ '4198.85 ms' │
└──────────────────────┴──────────────────────────────────────────┴──────────────┘

>> site-editor

┌──────────────────────┬──────────────────────────────────────────┬─────────────┐
│       (index)        │ 6d49143e3d63aed79b54ccc10ddac1918b432f79 │    trunk    │
├──────────────────────┼──────────────────────────────────────────┼─────────────┤
│    serverResponse    │               '115.13 ms'                │ '145.1 ms'  │
│      firstPaint      │               '388.67 ms'                │ '468.23 ms' │
│   domContentLoaded   │               '427.13 ms'                │ '513.47 ms' │
│        loaded        │                '590.1 ms'                │ '666.93 ms' │
│ firstContentfulPaint │               '388.67 ms'                │ '468.23 ms' │
│      firstBlock      │               '6572.97 ms'               │ '7006.6 ms' │
│         type         │                '39.62 ms'                │  '38.5 ms'  │
│       minType        │                '35.08 ms'                │ '34.38 ms'  │
│       maxType        │                '58.86 ms'                │ '59.57 ms'  │
└──────────────────────┴──────────────────────────────────────────┴─────────────┘

Problem

While experimenting with the windowing technique in #35230 we discovered some unexpected behavior with block focus time.

Focusing on a block in the main editor pane will re-render all ListViewBlock items. Depending on if list view is open or not, we can see some dramatic differences in how long it takes to resolve a focus event.

blocks focus time list view closed focus time list view open
50 87 ms 114ms
250 100 ms 311 ms
900 180 ms 1000 ms

If we profile using react devtools, here we can spot some slow renders on each block focus. Long story short, we query for selectedIds on the parent ListView component. In React if a parent updates all child components by default will update regardless of prop changes (which may or may result in DOM updates). So on focus, all items updated.

What we changed here was moving the selectedId querying to the child ListViewBlock component, which as we can see from the performance results above cleared up the slow render.

List View Will Re-render on block focus
137548678-7e3a2534-7253-4a7f-adb3-827340b2dd27

TODOs

  • Confirm logic changes around useListViewClientIds We never pass in selectedClientIds, so perhaps we can remove logic there.
  • Do we still need the appender code in List View? I didn't find any active references Appender code is okay to remove
  • Fix/confirm some logic for last of branch styling, and other misc classes.
  • Verify any differences between async/sync rendering. I ended up removing it in this PR since we no longer had selectedId in the branch to mark items to explicitly render sync Moved the nested async call to ListViewBlock
  • Can we update widget editor to use a sidebar for list view instead of a dropdown
  • Can we delete BlockNavigationDropdown, or is this needed for backwards compatibility? I opted to not remove the component in this PR

Folks are welcome to hop into the branch directly if you're aware of any needed functionality changes.

@github-actions
Copy link

github-actions bot commented Oct 16, 2021

Size Change: +281 B (0%)

Total Size: 1.07 MB

Filename Size Change
build/block-editor/index.min.js 135 kB -409 B (0%)
build/block-editor/style-rtl.css 13.9 kB -1 B (0%)
build/block-editor/style.css 13.9 kB -2 B (0%)
build/edit-widgets/index.min.js 16.3 kB +592 B (+4%)
build/edit-widgets/style-rtl.css 4.17 kB +51 B (+1%)
build/edit-widgets/style.css 4.18 kB +50 B (+1%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 931 B
build/admin-manifest/index.min.js 1.09 kB
build/annotations/index.min.js 2.7 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.08 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.2 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 470 B
build/block-library/blocks/button/editor.css 470 B
build/block-library/blocks/button/style-rtl.css 549 B
build/block-library/blocks/button/style.css 549 B
build/block-library/blocks/buttons/editor-rtl.css 309 B
build/block-library/blocks/buttons/editor.css 309 B
build/block-library/blocks/buttons/style-rtl.css 317 B
build/block-library/blocks/buttons/style.css 317 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 131 B
build/block-library/blocks/code/theme.css 131 B
build/block-library/blocks/columns/editor-rtl.css 206 B
build/block-library/blocks/columns/editor.css 205 B
build/block-library/blocks/columns/style-rtl.css 497 B
build/block-library/blocks/columns/style.css 496 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.17 kB
build/block-library/blocks/cover/style.css 1.17 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 977 B
build/block-library/blocks/gallery/editor.css 982 B
build/block-library/blocks/gallery/style-rtl.css 1.6 kB
build/block-library/blocks/gallery/style.css 1.59 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/home-link/style-rtl.css 247 B
build/block-library/blocks/home-link/style.css 247 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 502 B
build/block-library/blocks/image/style.css 505 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 488 B
build/block-library/blocks/navigation-link/editor.css 489 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/style-rtl.css 195 B
build/block-library/blocks/navigation-submenu/style.css 195 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.71 kB
build/block-library/blocks/navigation/editor.css 1.71 kB
build/block-library/blocks/navigation/style-rtl.css 1.7 kB
build/block-library/blocks/navigation/style.css 1.69 kB
build/block-library/blocks/navigation/view.min.js 2.74 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 198 B
build/block-library/blocks/page-list/style.css 198 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 347 B
build/block-library/blocks/post-comments-form/style.css 347 B
build/block-library/blocks/post-comments/style-rtl.css 475 B
build/block-library/blocks/post-comments/style.css 475 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 396 B
build/block-library/blocks/post-featured-image/editor.css 397 B
build/block-library/blocks/post-featured-image/style-rtl.css 156 B
build/block-library/blocks/post-featured-image/style.css 156 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 391 B
build/block-library/blocks/post-template/style.css 392 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 60 B
build/block-library/blocks/post-title/style.css 60 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 262 B
build/block-library/blocks/query-pagination/editor.css 255 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 250 B
build/block-library/blocks/separator/style.css 250 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 770 B
build/block-library/blocks/site-logo/editor.css 770 B
build/block-library/blocks/site-logo/style-rtl.css 165 B
build/block-library/blocks/site-logo/style.css 165 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/editor-rtl.css 824 B
build/block-library/blocks/social-links/editor.css 823 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB
build/block-library/blocks/social-links/style.css 1.32 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 571 B
build/block-library/blocks/video/editor.css 572 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 815 B
build/block-library/common.css 812 B
build/block-library/editor-rtl.css 9.62 kB
build/block-library/editor.css 9.62 kB
build/block-library/index.min.js 149 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/style-rtl.css 10.5 kB
build/block-library/style.css 10.5 kB
build/block-library/theme-rtl.css 668 B
build/block-library/theme.css 673 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/blocks/index.min.js 46 kB
build/components/index.min.js 212 kB
build/components/style-rtl.css 15.3 kB
build/components/style.css 15.3 kB
build/compose/index.min.js 10.4 kB
build/core-data/index.min.js 12.4 kB
build/customize-widgets/index.min.js 11.2 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 614 B
build/data/index.min.js 7.1 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 428 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.44 kB
build/edit-navigation/index.min.js 15.8 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29.4 kB
build/edit-post/style-rtl.css 7.12 kB
build/edit-post/style.css 7.12 kB
build/edit-site/index.min.js 30.7 kB
build/edit-site/style-rtl.css 5.71 kB
build/edit-site/style.css 5.71 kB
build/editor/index.min.js 37.6 kB
build/editor/style-rtl.css 3.78 kB
build/editor/style.css 3.77 kB
build/element/index.min.js 3.21 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 5.99 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.55 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.6 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.72 kB
build/keycodes/index.min.js 1.3 kB
build/list-reusable-blocks/index.min.js 1.85 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 845 B
build/nux/index.min.js 2.03 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.83 kB
build/primitives/index.min.js 921 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.63 kB
build/reusable-blocks/index.min.js 2.19 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.7 kB
build/server-side-render/index.min.js 1.52 kB
build/shortcode/index.min.js 1.48 kB
build/token-list/index.min.js 562 B
build/url/index.min.js 1.74 kB
build/viewport/index.min.js 1.02 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.11 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@gwwar
Copy link
Contributor Author

gwwar commented Oct 16, 2021

Initial results look promising for the focus case, manually profiling with the performance test post, focus time is back to ~180ms vs ~900ms

CleanShot 2021-10-16 at 10 46 17@2x

@gwwar gwwar added [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Type] Performance Related to performance efforts [Package] Block editor /packages/block-editor labels Oct 16, 2021
@gwwar gwwar self-assigned this Oct 16, 2021
@gwwar gwwar marked this pull request as ready for review October 16, 2021 21:04
selectedClientIds,
showOnlyCurrentHierarchy
) =>
const useListViewClientIdsTree = ( blocks, showOnlyCurrentHierarchy ) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about inlining this code? The hooks seem really simple now (which is great!)

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 do you mean moving the useSelect call to index.js or simplifying code in use-list-view-client-ids.js?

Comment on lines -142 to -151
{ hasAppender && (
<ListViewAppender
parentBlockClientId={ parentBlockClientId }
position={ rowCount }
rowCount={ appenderPosition }
level={ level }
terminatedLevels={ terminatedLevels }
path={ [ ...path, appenderPosition ] }
/>
) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I see it was mentioned in the PR description, was it removed because of performance issues?

The appender isn't mentioned in either of the tracking issues, so probably fine to remove. cc @priethor

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call, I don't think adding blocks directly in the list view is a top priority as it opens another whole can of worms, but we should at least have a separate discussion/issue; removing it here should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was it removed because of performance issues?

Yes, I was avoiding needing branch.js to query isSelected state. Some of the logic around the appender required isSelected state, and I didn't find any used references.

In general, if we have code that is unused, I'd prefer to remove it since it helps reduce package size and makes it easier to maintain. In the future we can always open a new PR to add the appender back.

Removing it here should be fine.

✨ Excellent!

showAppender &&
! isTreeRoot &&
isClientIdSelected( parentClientId, selectedClientIds );
const hasAppender = itemHasAppender( parentBlockClientId );
// Add +1 to the rowCount to take the block appender into account.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment could be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tidied in 489ad1b

Comment on lines 147 to 149
// 'is-last-of-selected-branch':
// withExperimentalPersistentListViewFeatures &&
// isLastOfSelectedBranch,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll investigate if we need these classes (and if we do I'll fix logic here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can fully remove this one, the border radius isn't even visible unless we add a background.

CleanShot.2021-10-18.at.14.10.26.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 401b409 👍


/**
* Internal dependencies
*/
import ListViewBlock from './block';
import ListViewAppender from './appender';
import { isClientIdSelected } from './utils';
import { useListViewContext } from './context';

export default function ListViewBranch( props ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the simplification of this component 🎉

// Make updates to the selected or dragged blocks synchronous,
// but asynchronous for any other block.
const isDragged = !! draggedClientIds?.includes( clientId );

return (
<AsyncModeProvider key={ clientId } value={ ! isSelected }>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the root of ListView still has AsyncModeProvider, so I think that would make the whole list async, even the selected block. Not seeing any regressions of #34519 though, so maybe it's ok ... I don't know why though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll try and tidy that. I mostly moved back to sync here, since we didn't have isSelected, and I know there are some visual glitches if we try rendering all items async.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving back to sync adds back some focus slowness. I instead moved the nested AsyncModeProvider call down to ListViewBlock in c344ca1


return __unstableGetClientIdsTree();
},
[ blocks, selectedClientIds, showOnlyCurrentHierarchy ]
[ blocks, showOnlyCurrentHierarchy ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I double-checked behavior in trunk and it looks like ListView is still being used as a dropdown in the widget editor.

Currently if showOnlyCurrentHierarchy is set, if an item is selected this will render a subset of the tree.

block selected with no blocks selected
CleanShot 2021-10-18 at 10 50 28@2x CleanShot 2021-10-18 at 10 50 22@2x

Can we use a sidebar instead in widgets editor? It feels like the popover has limited utility and that folks aren't very aware of this use case. (Note the scrollbars in trunk). If folks agree, I can create a separate PR or add on to this one.

cc @tellthemachines @draganescu @jasmussen

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love unifying on a sidebar, if nothing else then just to avoid drift. But I'll defer to @critterverse who may have thoughts on this.

Copy link
Contributor Author

@gwwar gwwar Oct 18, 2021

Choose a reason for hiding this comment

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

Tried this out in 1557543

CleanShot.2021-10-18.at.15.30.25.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! This could close #32311.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to adding it to the widgets screen!

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely yes!

@gwwar
Copy link
Contributor Author

gwwar commented Oct 18, 2021

Thanks for the review @talldan @jasmussen @priethor !

Scope here increased since I also tried displaying a ListView sidebar for edit-widgets. It'd be lovely to know if folks agree on the Widget ListView sidebar change, or if we should keep the dropdown.

Note that the sidebar change should also be easy to move that to a different PR. I'm aware that I also need to fix some E2Es from the widget change (I ran out of time today).

@draganescu
Copy link
Contributor

It'd be lovely to know if folks agree on the Widget ListView sidebar change, or if we should keep the dropdown.

I am all for removing the dropdown and replacing it with a sidebar! And apparent;ly so is everyone here #35706 (comment) 🎉

@gwwar gwwar force-pushed the try/list-view-decouple-is-selected branch from 1557543 to b453e31 Compare October 19, 2021 17:19
@gwwar
Copy link
Contributor Author

gwwar commented Oct 19, 2021

This one is ready for another review 👍 Two open questions:

Edit: since I wasn't sure I didn't include these changes.

@ramonjd
Copy link
Member

ramonjd commented Oct 19, 2021

I have a small PR going that replaces <Button /> list elements with <a /> elements (semantics and accessibility).

I rebased that PR on top of these changes and took it for a spin to check if anything stood out.

I couldn't spot any obvious regressions on desktop browsers.

In the Widget and Block Editors:

  • selecting block list items (nested and root level)
  • dragging and dropping block list items
  • mover control animations

all worked as expected. I also tested on iOS Safari.

Oct-20-2021 10-26-12

Also +1 for adding the sidebar. Thank you!!

@jasmussen
Copy link
Contributor

Just as a quick test, this is working well for me, seeing no regressions.

I did also try and stress it with drag/dropping in a list view with 900 blocks, and it completely choked, just like trunk, because this is not your windowing PR. So landing both of those would be just great, and if anyone reading this can help review and land them, we'll be in a vastly better place! 👌

@gwwar gwwar force-pushed the try/list-view-decouple-is-selected branch from 4d131a3 to 28f60b5 Compare October 22, 2021 16:33
@gwwar
Copy link
Contributor Author

gwwar commented Oct 22, 2021

@priethor was kind enough to look over the changelog notes, so this one is all set ✨ Thanks for the reviews folks!

@gwwar gwwar merged commit 30689b8 into trunk Oct 22, 2021
@gwwar gwwar deleted the try/list-view-decouple-is-selected branch October 22, 2021 17:16
@github-actions github-actions bot added this to the Gutenberg 11.9 milestone Oct 22, 2021
@jasmussen
Copy link
Contributor

🔥

@gwwar gwwar changed the title List View: avoid re-rendering all items on block focus List View: avoid re-rendering all items on block focus. Enable persistent List View in the widget editor. Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Package] Block editor /packages/block-editor [Package] Edit Widgets /packages/edit-widgets [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants