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

Added missing reference to lodash #14516

Merged
merged 1 commit into from
Mar 20, 2019
Merged

Conversation

jakeparis
Copy link
Contributor

A reference to the omit method of lodash is in the Changing the innerBlocks section, but the method is not defined in the code examples, causing confusion.

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

A reference to the `omit` method of lodash is in the *Changing the innerBlocks* section, but the method is not defined in the code examples, causing confusion.
@@ -194,7 +194,8 @@ E.g: a block wants to migrate a title attribute to a paragraph innerBlock.
{% ES5 %}
```js
var el = wp.element.createElement,
registerBlockType = wp.blocks.registerBlockType;
registerBlockType = wp.blocks.registerBlockType,
omit = lodash.omit;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if our documentation should rely on the lodash global variable being available. It would potentially block us from one day WordPress removing lodash. Maybe a better option would be to refactor the sample code to avoid the need for omit function usage?
Any thoughts on this @gziolo, @youknowriad, @aduth?

Copy link
Member

Choose a reason for hiding this comment

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

It could be okay to refactor, though I also don't see Lodash going anywhere anytime soon, if ever, as it's offered as a publicly-available script handle in core.

Copy link
Member

Choose a reason for hiding this comment

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

We use also lodash in another example. I will land this change as it fixes an issue. If you have some good idea how to improve it further, let’s proceed with follow-up PR.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

See my other comment 👍

@gziolo gziolo merged commit fb8abfe into WordPress:master Mar 20, 2019
@youknowriad youknowriad added this to the 5.4 (Gutenberg) milestone Mar 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants