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

Pass content to dynamic block render functions #1340

Merged
merged 1 commit into from
Jul 9, 2017

Conversation

westonruter
Copy link
Member

Currently only the attributes are passed to the PHP render function. This PR changes that to allow the contents of the block also to be passed. This brings blocks closer aligned with shortcodes, for which the callback functions take two arguments, the attributes and the content.

@youknowriad
Copy link
Contributor

The reason why the content was not passed is because the render function is needed for dynamic blocks (server side rendered) and their content is not "important" and is considered a fallback content only.

But I guess, this addition does no harm?
Do you think of real use-cases where the content would be used in the render function?

@westonruter
Copy link
Member Author

One use case could be for a dynamic block reading content would be for something like a chart block which has content that is CSV data that is needed for rendering the chart and accompanying table on the server.

@youknowriad
Copy link
Contributor

To be clear, I'm not against this I'm just trying to figure out when will we use this exactly.

That said, I think the content should always be HTML, we can't really store CSV there and a JSON seems a better approach for this.

@westonruter
Copy link
Member Author

OK, an HTML table then can be the content 😄

I got a separate request for this in a DM in Slack. I've asked them to respond here with the use case as well.

@willybahuaud
Copy link
Contributor

willybahuaud commented Jun 21, 2017

Hello!

Here is an use case, using dropCap with the text bloc:

<!-- wp:core/text dropCap="true" -->
<p>fzefqzfqzf</p>
<!-- /wp:core/text -->

@westonruter
Copy link
Member Author

So in that case, the PHP rendering function would inject a dropCap class name on the first <p> or wrap the block's contents in a div element that can be targeted by CSS to apply the style.

@westonruter westonruter force-pushed the update/pass-content-to-php-render-function branch from 86b1ee8 to 2ca96f9 Compare July 9, 2017 07:17
@westonruter
Copy link
Member Author

Rebased to fix merge conflict. Former HEAD was 86b1ee8.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 17.816% when pulling 2ca96f9 on update/pass-content-to-php-render-function into 4564ee0 on master.

@youknowriad
Copy link
Contributor

youknowriad commented Jul 9, 2017

For consistency, It's probably better to pass the content to the render function but for the dropCap use case, it should be done in the save function IMO

@westonruter westonruter merged commit 02c9f1b into master Jul 9, 2017
@westonruter westonruter deleted the update/pass-content-to-php-render-function branch July 9, 2017 14:48
youknowriad pushed a commit that referenced this pull request Jan 17, 2020
youknowriad pushed a commit that referenced this pull request Jan 17, 2020
…rg-mobile into add/autosave-monitor

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (56 commits)
  Update gutenberg ref
  Update gutenberg ref
  Update gutenberg ref
  Update gutenberg ref
  Add support for group component (#1335)
  Update gutenberg hash
  fix error generating app name on sauce
  Bump eslint-utils from 1.3.1 to 1.4.2
  Update gutenberg ref
  Update release notes
  Update gutenberg ref to rnmobile/master HEAD
  disable paste tests
  MediaUpload and MediaPlaceholder unify props (#1310)
  Update gutenberg ref (#1340)
  Update gutenberg ref
  disable list tests and enable paste tests
  enable list tests
  enable list end tests
  enable block insertion tests
  enable heading tests
  ...

# Conflicts:
#	gutenberg
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants