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

Move Reusable Block definition from blocks module to editor module #3967

Closed

Conversation

noisysocks
Copy link
Member

@noisysocks noisysocks commented Dec 13, 2017

Inspired by this comment by @youknowriad, I though I'd try moving the Reusable Block block definition from the blocks module to the editor module.

The benefit of doing this is:

  • It illustrates that core/block is tightly coupled to the redux state that lives inside the editor module.
  • We are able to re-use the selectors and actions defined in editor/selectors.js and editor/actions.js.

I chose editor/library as the place where these tightly coupled block definitions live so as to closely mirror blocks/library. Happy to change this though.

To test:

  1. Convert a static block to a reusable block
  2. Convert a reusable block to a static block
  3. Edit and save a reusable block
  4. Insert a reusable block
  5. Preview a post with a reusable block in it

cc. @youknowriad @aduth

By having the definition in the `editor` module, we are able to easily
access the redux store using the selectors and actions defined in
`editor/selectors.js` and `editor/actions.js`.
@youknowriad
Copy link
Contributor

I actually think all the blocks should be defined outside of the "blocks" module which should contain only the "API" and the "reusable pieces" :)

What about the stylesheets? should we update the webpack config to extract the block styles properly from this new folder (separate style.scss and editor.scss same as we do in the blocks module). Though I wonder where "editor/library/block/edit-panel/style.scss" would have been loaded, it should be loaded in the editor's stylesheet but I wonder if it works because its name is "style.scss"

@noisysocks
Copy link
Member Author

Though I wonder where "editor/library/block/edit-panel/style.scss" would have been loaded

We import that SCSS file into ReusableBlockEditPanel:

And our CSS loader will pick up anything with the .scss extension:

gutenberg/webpack.config.js

Lines 124 to 130 in f1fc7b6

{
test: /\.s?css$/,
exclude: [
/blocks/,
],
use: mainCSSExtractTextPlugin.extract( extractConfig ),
},

@youknowriad
Copy link
Contributor

@noisysocks I guess since we do not have any "frontend" block style in the "editor" module, this would work right now but if ever we introduce some of these styles later, this would break. I'm ok leaving this for later.

@youknowriad
Copy link
Contributor

youknowriad commented Dec 18, 2017

maybe editor/blocks instead of editor/library. The thinking here is that "library" doesn't indicate that we're defining blocks.

This works well for me. Any thoughts @aduth @gziolo

@gziolo
Copy link
Member

gziolo commented Dec 18, 2017

Shouldn't we treat Reusable Block as another edit mode and move the handling to <BlockListBlock /> instead? At the moment it's promoted to its own block type which nests another block type. It isn't bad itself but leads to a few subtle bugs that are getting more prominent as we move next pieces to extensibility layer.

I discovered 3 bugs when working on #4009:

  1. Reusable Block doesn't get wrapper props applied so some styles don't get properly applied:
    screen shot 2017-12-18 at 10 36 05
  2. Edit as HTML doesn't work properly:
    screen shot 2017-12-18 at 10 40 24
  3. The anchor setting isn't added to the Block Settings, which means that filters weren't applied, the class name setting comes from the Reusable Block (not from the Heading block):
    screen shot 2017-12-18 at 10 41 41

I'm not convinced what it the best solution, but it definitely needs some further thinking how to tackle it to make it easier to maintain together with other moving pieces.

@gziolo
Copy link
Member

gziolo commented Dec 18, 2017

I actually think all the blocks should be defined outside of the "blocks" module which should contain only the "API" and the "reusable pieces" :)

This is a quite interesting idea. I'm sure they shouldn't be bootstrapped as part of importing wp.blocks. It should happen when Editor or everything else needs them to render content. Where their definition should leave is another question. I don't have a good answer to that question.

@youknowriad
Copy link
Contributor

Edit as HTML doesn't work properly:

Reusable blocks should just disable the HTML support IMO

@aduth aduth added the [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) label Dec 19, 2017
@noisysocks
Copy link
Member Author

Shouldn't we treat Reusable Block as another edit mode and move the handling to <BlockListBlock /> instead?

I think this is worth exploring. I've also noticed some weird behaviour with using the keyboard to move between and edit Reusable Blocks. It might be that our approach of keeping everything compartmentalised within the core/block component is untenable.

@gziolo
Copy link
Member

gziolo commented Dec 22, 2017

Another issue related to Reusable Blocks I stumbled upon today:

screen shot 2017-12-22 at 11 04 02

The wrapper component gets added to the recent blocks list. It isn't useful without nested block...

@youknowriad, @aduth: How do you feel about the idea of refactoring Reusable Blocks as an editor feature build upon hooks API? Let's say each block could opt out of being reusable using:

support: {
    reusable: false,
},

We would detect if the block is set as reusable using existing connect logic and wrap BlockEdit with all needed wrapper code using blocks.BlockEdit hook. We would also have to completely override save method using what the block implementation has at the moment. I guess there is more work necessary, but it would be an excellent opportunity to confirm that our extensibility API is robust enough.

@noisysocks
Copy link
Member Author

Another issue related to Reusable Blocks I stumbled upon today: The wrapper component gets added to the recent blocks list. It isn't useful without nested block...

This bug was filed here: #3793

@noisysocks
Copy link
Member Author

Going to close this PR for now as it's very stale. Let's revisit this later.

@noisysocks noisysocks closed this Jan 29, 2018
@noisysocks noisysocks deleted the try/move-reusable-block-definition-to-editor branch January 29, 2018 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants