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

Align tutorial with example plugin repo code #12740

Closed
wants to merge 4 commits into from
Closed

Align tutorial with example plugin repo code #12740

wants to merge 4 commits into from

Conversation

terriann
Copy link
Contributor

@terriann terriann commented Dec 9, 2018

Description

Contributing to #11251 from contributor day at WCUS18

I've updated the code references to more closely match the referenced example plugin repo https://github.com/WordPress/gutenberg-examples

After discussion with @0aveRyan we decided to strip the internationalization from the code when outlining the tutorial. A note was added below the code samples to indicate this, but if there is a better suggestion I'm happy to help make changes.

Any other feedback & change requests please ping me - rookie contributor so I'm sure we will need to iterate :)

How has this been tested?

Documentation changes only - not tested.

Screenshots

n/a

Types of changes

  • Documentation updates

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added the [Type] Developer Documentation Documentation for developers label Dec 14, 2018
@gziolo gziolo added this to the Documentation & Handbook milestone Dec 14, 2018
Copy link
Contributor

@chrisvanpatten chrisvanpatten left a comment

Choose a reason for hiding this comment

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

One small tweak. I'd love someone to review the ES5 syntax but the ES6 looks good to me.

Admittedly I'm slightly uncomfortable omitting i18n. I get why (it adds a lot of extra levels to what should be a simple tutorial), but we have to assume people are likely to copy and paste this code and it should, where possible, be an example of the right way, tip-to-toe.

That said the tutorial didn't previously have i18n, so the addition of the note is a welcome improvement from master. I'd be happy to land as-is, then have a separate discussion on i18n in tutorials at a later point.


function onChangeContent( newContent ) {
edit: ( props ) => {
const { attributes: { content }, setAttributes, className } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this split across newlines for clarity, e.g.

const {
  attributes: {
    content,
  },
  setAttributes,
  className,
} = props;

(with tabs instead of spaces)

@ajitbohra
Copy link
Member

It would be good to have i18n in examples, we can links users to full working examples and i18n docs for further reference. Also, https://github.com/WordPress/gutenberg-examples need an update for i18n implementation.

@gziolo gziolo requested review from mkaz and oandregal January 31, 2019 14:36
@oandregal
Copy link
Member

@terriann I see that there are conflicting changes in some files. Would you mind integrating those changes and then ping us for review again?

var RichText = editor.RichText;
var AlignmentToolbar = editor.AlignmentToolbar;
var BlockControls = editor.BlockControls;

Copy link
Member

Choose a reason for hiding this comment

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

I prefer passing in wp as the global, because than you can use additional parts without having to change what is being passed in, or a mistake in the order of items passed in.

( function( wp ) {
	var el = wp.element.createElement;
	var RichText = wp.editor.RichText;
	var AlignmentToolbar = wp.editor.AlignmentToolbar;
	var BlockControls = wp.editor.BlockControls;

window.wp.blocks,
window.wp.editor,
window.wp.element
) );
Copy link
Member

Choose a reason for hiding this comment

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

See comment above to switch this to just ( window.wp )

( function( blocks, editor, element ) {
var el = element.createElement;
var RichText = editor.RichText;

Copy link
Member

Choose a reason for hiding this comment

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

Same as previous file, using ( function( wp ) {

@youknowriad youknowriad removed this from the Documentation & Handbook milestone Mar 18, 2019
@oandregal
Copy link
Member

Closing this in favor of #14584

@oandregal oandregal closed this Mar 27, 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.

7 participants