Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Support BlockContext between different blocks #7

Merged
merged 11 commits into from
May 24, 2022
Merged

Conversation

luisherranz
Copy link
Member

The idea is to try using a custom event that is captured by a parent block that populates the context.

Inspired by preact-custom-element.

@luisherranz
Copy link
Member Author

I've added a small explanation of how this could work in the last summary video: #6 (comment) (minute 6:30)

@michalczaplinski michalczaplinski force-pushed the context-experiments branch 2 times, most recently from 5c467a2 to f4a3a28 Compare May 18, 2022 23:36
@michalczaplinski michalczaplinski changed the base branch from main to feature/opt-out-frontend-attributes May 18, 2022 23:37
@michalczaplinski
Copy link
Collaborator

Sorry, for the triple force-push - I've changed the base of this PR (to include the fix in #19) and messed up the local branch in the process. All fixed now 🙂 .

@luisherranz
Copy link
Member Author

No problem. I've merged main to keep it in sync with the latest changes (mainly formatting) 👍

@luisherranz
Copy link
Member Author

I'm now the one who has messed something up 😆

@luisherranz luisherranz force-pushed the context-experiments branch from dcd361f to bc47e20 Compare May 19, 2022 10:21
@luisherranz
Copy link
Member Author

Rebasing is not working either. Main commits are still appearing 😕

@luisherranz
Copy link
Member Author

Ohhhh, ok. I got it. It's because you changed the base. I'll change it back to main.

@luisherranz luisherranz changed the base branch from feature/opt-out-frontend-attributes to main May 19, 2022 10:23
@luisherranz
Copy link
Member Author

Great. Everything back to normal 🙂

@luisherranz
Copy link
Member Author

@michalczaplinski, are you working on this?

@michalczaplinski
Copy link
Collaborator

Yes! I don't have it in a workable state yet, but I ll share an update later today 🙂

@michalczaplinski
Copy link
Collaborator

michalczaplinski commented May 20, 2022

Below's the current status update in video form.

The code is "working" but I didn't make much effort to clean it up yet.

There is also an issue that I have mentioned in the video, which I ll document later on. When including the parent block includes only the child block, it does not render that child block. I think this might be an issue with rendering inner blocks that was introduced in #8.

https://www.loom.com/share/a0f0d917491e471990d36d62da6cbb1e

Comment on lines +4 to +11
// We assign `blockTypes` to window to make sure it's a global singleton.
//
// Have to do this because of the way we are currently bundling the code
// in this repo, each block gets its own copy of this file.
//
// We COULD fix this by doing some webpack magic to spit out the code in
// `gutenberg-packages` to a shared chunk but assigning `blockTypes` to window
// is a cheap hack for now that will be fixed once we can merge this code into Gutenberg.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the comment I explain why I'm doing window.blockTypes (it's a hack).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this should be part of the Gutenberg APIs, so we can keep the hack for now 🙂

Comment on lines -63 to +117
customElements.define( 'gutenberg-interactive-block', GutenbergBlock );
// We need to wrap the element registration code in a conditional for the same
// reason we assing `blockTypes` to window (see top of the file).
//
// We need to ensure that the component registration code is only run once
// because it throws if you try to register an element with the same name twice.
if ( customElements.get( 'gutenberg-interactive-block' ) === undefined ) {
customElements.define( 'gutenberg-interactive-block', GutenbergBlock );
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Directing your attention to the comment 🙂

@michalczaplinski
Copy link
Collaborator

michalczaplinski commented May 21, 2022

@luisherranz I can't assign you for review because you've opened the PR 🙂
@DAreRodz can't assign you either (maybe cause you're not a collaborator? 🤔)

@michalczaplinski
Copy link
Collaborator

There is also an issue that I have mentioned in the video, which I ll document later on. When including the parent block includes only the child block, it does not render that child block. I think this might be an issue with rendering inner blocks that was introduced in #8.

I've managed to debug it and opened a new issue for this: #23

@cbravobernal cbravobernal requested a review from DAreRodz May 23, 2022 07:41
Copy link
Collaborator

@cbravobernal cbravobernal left a comment

Choose a reason for hiding this comment

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

Nice job!

Copy link
Member Author

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

I can't approve my own PR.

This is fantastic, Michal! 😄👏

I've added a couple of questions, but it's looking great. I'm glad this approach seems to work fine.

If you don't mind, I'm going to rename the blocks as I did in #25.

EDIT: I've also moved the frontend components to each block folder to make the block structure clearer.


The next step would probably be supporting dynamic contexts. I'll add a task for that but basically, we need to make this work:

// Parent provides attributes.count as context
const ParentFrontend = ({ attributes, setAttributes }) => {
  return (
    <button
      onClick={() => {
        setAttributes({ count: attributes.count + 1 });
      }}
    >
      increment context
    </button>
  );
};

// Children is rerendered when context changes (on the parent click)
const ChildrenFrontend = ({ context }) => (
  <div>The count is: {context.count}</div>
);

EDIT: I've added a new task for this.

Comment on lines +1 to +5
// This import is needed to ensure that the `wp.blockEditor` global is available
// by the time this component gets loaded. The `Title` component consumes the
// global but cannot import it because it shouldn't be loaded on the frontend of
// the site.
import '@wordpress/block-editor';
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you explain this a little bit? This block doesn't use the Title component, does it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, it seems that it does work fine even if I remove the import '@wordpress/block-editor'; which makes sense.

I have added it because it seemed make the warning about wp.RichText go away, but looks like I don't need it after all.

if ( el !== null ) {
// listen for the ping from the child
el.addEventListener( 'gutenberg-context', ( event ) => {
event.stopPropagation();
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any reason to stop the propagation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes! I explain below in the video 🙂

ref={( el ) => {
if ( el !== null ) {
// listen for the ping from the child
el.addEventListener( 'gutenberg-context', ( event ) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not saying this is right or wrong, but what's your reasoning for adding the event listener to the guntenberg-inner-blocks instead of the block wrapper? 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this is the only option we have if I'm not mistaken. Lemme explain in a video:

2022-05-23_20-24-04.mp4

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thanks for the video, Michal 🙂

I think those problems can be circumvented, but we shouldn't worry for now, this is perfectly fine as it is. If there's a simpler solution, it will present itself once we do more complex stuff!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for completeness, I also thought we could wrap the Comp like in a "useless" div like:

<div
  style={{ display: 'contents' }}
  ref={el => { /* add the eventListener here */ } }
>
  <Comp
    attributes={attributes}
    blockProps={blockProps}
    suppressHydrationWarning={true}
    context={context}
  >
    <Children
      value={innerBlocks && innerBlocks.innerHTML}
      suppressHydrationWarning={true}
    />
  </Comp>
</div>
					

but that doesn't work well - it was e.g. breaking some styles. And that <div> does not have the blockProps applied to it which can cause other problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be the benefit of using an extra div versus using this as you explain in the video?

this points to the <gutenberg-interactive-block> wrapper.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There would be no benefit, that's what I was trying to say. But the issue is that you only want to ask the parent that is one level up for the context:

I've realized that we could also do this:

// inside of the connectedCallback()

this.addEventListener( 'gutenberg-context', ( event ) => {
  if ( this !== event.target ) { // <- only set the context if we're in the parent. 
    event.stopPropagation();
    event.detail.context = providedContext;
  }
} );

// ping the parent for the context
const event = new CustomEvent( 'gutenberg-context', {
  detail: {},
  bubbles: true,
  cancelable: true,
} );

this.dispatchEvent( event );

So, this way we can both listen to and fire the event on the same element (the <interactive-gutenberg-block>) but only set the context if we're in the parent. This works because the event bubbles up.

Was this what you were suggesting or did you have another idea in mind?

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, I think so 🙂

I think you could also add the event listener after you've dispatched the event so they don't interfere.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants