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

Block Editor: Rename block context in BlockListBlock #21922

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 27, 2020

Description

Fixes #21664.

As of #21467, there are now three React Context objects related to blocks:

  • BlockContext in block-editor/src/components/block-list/block.js
  • BlockEditContext in block-editor/src/components/block-edit/context.js
  • BlockContext in block-editor/src/components/block/context/index.js

For fear of potential confusion, these should each have distinct names. Currently, there is conflict between the two "BlockContext" objects. The latter of these is responsible for managing the feature of the same name ("Block Context", implemented in #21467). The former is specific to the context of a block within the block list. As such, it is more easily renamed.

Task:

Rename BlockContext in block-list/block.js to BlockListContext or BlockListBlockContext.

@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Block editor /packages/block-editor labels Apr 27, 2020
@gziolo gziolo requested review from aduth and ellatrix April 27, 2020 13:22
@gziolo gziolo requested a review from youknowriad as a code owner April 27, 2020 13:22
@gziolo gziolo self-assigned this Apr 27, 2020
@github-actions
Copy link

github-actions bot commented Apr 27, 2020

Size Change: 0 B

Total Size: 816 kB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.23 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 106 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.03 kB 0 B
build/block-library/editor.css 7.03 kB 0 B
build/block-library/index.js 114 kB 0 B
build/block-library/style-rtl.css 7.14 kB 0 B
build/block-library/style.css 7.14 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 27.6 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 10.9 kB 0 B
build/edit-site/style-rtl.css 5.11 kB 0 B
build/edit-site/style.css 5.11 kB 0 B
build/edit-widgets/index.js 7.49 kB 0 B
build/edit-widgets/style-rtl.css 4.67 kB 0 B
build/edit-widgets/style.css 4.66 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.4 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth
Copy link
Member

aduth commented Apr 27, 2020

I think the consumer usage needs to be updated, since we changed the exported name.

import { BlockContext } from './block';

@gziolo
Copy link
Member Author

gziolo commented Apr 27, 2020

I don’t know how I missed this export 😅

@aduth
Copy link
Member

aduth commented Apr 27, 2020

I don’t know how I missed this export 😅

It seemed strange to me that we were changing a provider name, and there were no changes for consumers 😛

@gziolo gziolo force-pushed the update/block-list-block-context branch from 77e876f to 286281c Compare April 28, 2020 05:50
@youknowriad
Copy link
Contributor

Unrelated to this PR but I've been thinking about whether we really need this context at all since both these two components (provider and consumer) are internal components that have access to clientID so they can figure out all this data inline. Also wondering if all these properties are used in both or just in the wrapper (so just move them there)

@aduth
Copy link
Member

aduth commented Apr 28, 2020

Unrelated to this PR but I've been thinking about whether we really need this context at all since both these two components (provider and consumer) are internal components that have access to clientID so they can figure out all this data inline. Also wondering if all these properties are used in both or just in the wrapper (so just move them there)

It's an interesting point. I'm not aware of the background here. I could see how the provider could be useful in that these values are "already available" at the top-level block, so passing the value down through context could avoid needing to select them a second time. But in practice, I'm not sure how much value we get from this, since the memoization itself would have some overhead (Object.values and iterating to compare an array of 15 members on every block's render), at least considering that as of today, only one block (the Group block) actually uses <BlockWrapper />.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Despite conversation above, the changes do satisfy the needs of the issue.

@gziolo gziolo merged commit 2133b25 into master Apr 28, 2020
@gziolo gziolo deleted the update/block-list-block-context branch April 28, 2020 14:06
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone Apr 28, 2020
@gziolo
Copy link
Member Author

gziolo commented Apr 28, 2020

@youknowriad, as soon as it’s confirmed that we can skip using the context here, I’m happy to open another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Editor: BlockList: Rename BlockContext as BlockListContext
3 participants