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

Make calendar block resilient against editor module not being present #16161

Conversation

jorgefilipecosta
Copy link
Member

Description

The block should not assume the editor store is available.

How has this been tested?

I added a calendar block to one of the widget areas.
I switched to this branch and cherry-picked #16160.
I verified the calendar block still works. I go to branch #16160 without these changes the block crashes.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Block] Calendar Affects the Calendar Block labels Jun 13, 2019
@@ -61,9 +61,13 @@ class CalendarEdit extends Component {
}

export default withSelect( ( select ) => {
const coreEditorSelect = select( 'core/editor' );
Copy link
Member

Choose a reason for hiding this comment

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

This is very concerning as it might break many 3rd party blocks when they are inserted. Do you have any issue where you track discussion related to how to ensure that blocks are rendered in the proper context?

Copy link
Member

Choose a reason for hiding this comment

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

What if other block explicitly imports core/editor package making this store available?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if other block explicitly imports core/editor package making this store available?

It may not break the block, but might produce weird results. For instance, there's no post object or something like that. This is one of those things we need to document properly before the release. It's still on the table that the blocks should opt-in or opt-out of the widgets screen. I think we need more wide usage of the widgets screen here before making an informed decision.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, some blocks that rely on the editor module may not work on the widget screen, because we don't have a post there. There is not exactly breaking back-compatibility because these blocks were created for the post editor and they still work on the post editor; I guess an option to disable/enable blocks from the widget screen may be a possibility.

@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jun 14, 2019
@youknowriad
Copy link
Contributor

I added the "Needs dev note" label, that way we don't forget documenting this before the WP release including these changes.

@jorgefilipecosta jorgefilipecosta merged commit 4f5b472 into master Jun 14, 2019
@jorgefilipecosta jorgefilipecosta deleted the fix/make-calendar-block-resilient-against-editor-not-being-present branch June 14, 2019 10:57
daniloercoli added a commit that referenced this pull request Jun 14, 2019
…rnmobile/open-video-by-browser-for-android

* 'master' of https://github.com/WordPress/gutenberg: (34 commits)
  [RNMobile] Native mobile release v1.7.0 (#16156)
  Scripts: Document and simplify changing file entry and output of build scripts (#15982)
  Block library: Refactor Heading block to use class names for text align (#16035)
  Make calendar block resilient against editor module not being present (#16161)
  Bump plugin version to 5.9.1
  Editor: Fix the issue where statics for deprecated components were not hoisted (#16152)
  Build Tooling: Use "full" `npm install` for Build Artifacts Travis task (#16166)
  Scripts: Fix naming conventions for function containing CLI keyword (#16091)
  Add native support for Inserter (Ported from gutenberg-mobile) (#16114)
  docs(components/higher-order/with-spoken-messages): fix issue in example code (#16144)
  docs(components/with-focus-return): fix typo in README.md (#16143)
  docs(block-editor/components/inspector-controls): fix image path in README.md (#16145)
  Add mention of on Figma to CONTRIBUTING.md (#16140)
  Bring greater consistency to placeholder text for media blocks. (#16135)
  Add descriptive text and a link to embed documentation in embed blocks (#16101)
  Update toolbar-text.png (#16102)
  Updating changelogs for the Gutenberg 5.9 packages release
  chore(release): publish
  [RNMobile] Fix pasting text on Post Title (#16116)
  Bump plugin version to 5.9.0
  ...

# Conflicts:
#	packages/block-library/src/video/video-player.android.js
@youknowriad youknowriad added this to the Gutenberg 6.0 milestone Jun 24, 2019
@youknowriad
Copy link
Contributor

Since the widgets screen is not released yet, let's delay this dev note :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Calendar Affects the Calendar Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants