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

Enhancement: Improve Documentation #278

Closed
wants to merge 338 commits into from

Conversation

ahmadawais
Copy link
Contributor

This PR improves the internal documentation for index.php

ellatrix and others added 30 commits February 28, 2017 11:45
…ui-prototype

Prevent input for non-text selections
…ui-prototype

Use functions instead of commands where appropriate
…tyle. Also make it keyboard friendly (WordPress#136)

* update the TinyMCE per Block prototype UI to match the Prototype UI style
* Adding heading block
* Adding a paragraph block and moving to React
 - preact has some state updating bugs
* Allowing removing/merging components using backspace
* Handling navigation using arrow keys
* Adding a simple quote  block
- Includes a new drag and drop mockup
- Includes an admin with the sidebar closed
- Includes an updated Sketch file that has the admin UI stuff
…ui-prototype

Tinymce single/mimic ui prototype
…ui-prototype

Hide inline formatting toolbar along with rest of block UI
…ui-prototype

Tinymce single/mimic ui prototype
…ui-prototype

Add undo levels for moving blocks
…ui-prototype

Lighter UI: only leave align centre, clearer remove format icons
ellatrix and others added 14 commits March 15, 2017 08:40
Note: It creates additional paragraphs when dragging non editable blocks.

This reverts commit bef6be3.
These mockup updates include general refinements and polish, per Github and Slack discussions. Notably around:

- Galleries. Props @j-falk for ideas here. Trash and edit buttons when selecting individual images in a gallery, and button for advanced gallery editing, where you'd also be able to add more images to the gallery, or rearrange, in the case of a slideshow version of the gallery.
- Drag and drop. Props @iseulde for the idea of showing a blue line for the dropzone. The gray area is a ghost of where you're dragging from, so the blocks don't size around when you drag.
- A new approach to inserting content. It generally happens on a newline, and the plus then shows up to the left, where the up/down arrows would show up for a non-empty block. We'd then have a permanent "insert" button somewhere in the Admin UI to go with this, for when you need to insert content between two letters. See also WordPress#78.
- Added empty versions of Quote and Image block. Imagine the use case of _type type type, press enter for newline, type `/ima` to open inserter searched to Image, press enter, arrow down, type type type_. In that case you'd simply inserted an image placeholder block which you could later upload to, or pick an image from the media library.
Filenames with unencoded spaces worked yesterday. Suddenly it doesn't. No idea why. Will use dashes instead of spaces in the future.
This PR improves the internal documentation for index.php
/**
* Plugin Name: Gutenberg
* Plugin URI: https://wordpress.github.io/gutenberg/
* Description: Prototyping since 1440. Development plugin for the editor focus in core.
* Version: 0.1
*/

// Hook.
Copy link
Member

Choose a reason for hiding this comment

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

My personal view is that a comment shouldn't exist to describe what exists, but why (more specifically in clarifying potentially ambiguous logic). In this case, "Hook" is already quite clear merely by the invoking add_action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention of adding this comment is always to be liberal with documentation. It not only makes sense (to me) but helps the code look more clean.

Copy link
Member

@aduth aduth Mar 16, 2017

Choose a reason for hiding this comment

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

My intention of adding this comment is always to be liberal with documentation.

There's an underlying assumption here that "more documentation is always better", to which my comment is primarily directed at disputing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting that we should not document this? Do you have an alternate suggestion here?

Copy link
Member

Choose a reason for hiding this comment

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

Are you suggesting that we should not document this? Do you have an alternate suggestion here?

Right, it's fine to not have comments where comments aren't serving value.

/**
* Gutenberg's Menu.
*
* @since 0.0.1
Copy link
Member

Choose a reason for hiding this comment

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

Do you think @since should follow the repository's own versioning, or that of what we expect the WordPress version to be upon integration? We may simply want to leave it out for future consideration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, currently, I added it as per the repository's version but changing it to 4.8.0 does make more sense. Will go ahead and change it.

plugin/index.php Outdated
/**
* Scripts & Styles.
*
* @param [type] $hook
Copy link
Member

Choose a reason for hiding this comment

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

[type] should be defined. There should also be a description explaining what $hook is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not as clear to me, so I let it be to be reviewed and added at a later stage. I think the $hook is going to be a string value of screen name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just addressed this one.

@@ -19,13 +23,26 @@ function gutenberg_menu() {
);
}

// Hook.
add_action( 'admin_enqueue_scripts', 'gutenberg_scripts_and_styles' );
Copy link
Member

Choose a reason for hiding this comment

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

I like that this was made consistent with positioning relative to its callback function with the one earlier in this file, but what's the pattern we should follow with regards to being above/below the function declaration? At least in twenty* themes and built-in plugins (hello.php), it's placed below the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an intentional decision. See this:
image 2017-03-16 at 6 59 27 pm public

Having function and file level DocBlocks right after one and another makes less sense. That's why I kept the hooks at the top.

Copy link
Member

Choose a reason for hiding this comment

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

Having function and file level DocBlocks right after one and another makes less sense.

In what way does it make less sense?

The relative positioning doesn't really impact much since it works in either case. My point in bringing it up was in whether it may be at odds with expectations other contributors would have coming in being familiar with differing patterns found more commonly in other plugins and core code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if there is a pattern there. I myself follow both, but I remain consistent at least in one single project.

It makes lesser sense as two block level comments stacked together do NOT look good. That's the only reason why I do this.

Normally, I creat a function like this.

Copy link
Member

Choose a reason for hiding this comment

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

Here's a couple examples of the precedent I was referring to:

There's not a whole lot of examples in the core code itself, as callbacks are often defined separate from the hook itself or the hook is created in the constructor of a class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess, that's what I ended up with on the merged PR>

@aduth
Copy link
Member

aduth commented Mar 16, 2017

Definitely good to be thinking about documenting functions. We should be striving to include these with any new code we introduce 👍

@ahmadawais
Copy link
Contributor Author

Just addressed the conflicts. See if this is mergable now?

@youknowriad
Copy link
Contributor

I think this needs to be rebased over the new master (to avoid keeping the old history)

@ahmadawais
Copy link
Contributor Author

@youknowriad Should I add a new PR or rebase this one?

@youknowriad
Copy link
Contributor

What works best for you. I'm not an expert Git, if you want to keep this one, maybe @nylen could help

@ahmadawais ahmadawais closed this Mar 17, 2017
@ahmadawais
Copy link
Contributor Author

@aduth and @youknowriad I have closed this PR. Was going to rebase it but I think the plugin files have been moved since then? Do you know where?

@ahmadawais ahmadawais deleted the patch-4 branch March 17, 2017 13:34
@youknowriad
Copy link
Contributor

yes, the root folder :)

@ahmadawais
Copy link
Contributor Author

Damn, how did I miss that :P 👍

@aduth
Copy link
Member

aduth commented Mar 17, 2017

Sorry for the delay in responding. I'll add comments here, though understanding points have been reiterated and changes merged in #290.

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.

7 participants