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

Author Select #328

Merged
merged 13 commits into from
Feb 15, 2018
Merged

Author Select #328

merged 13 commits into from
Feb 15, 2018

Conversation

liamdefty
Copy link
Contributor

To be merged after milestone/1.7

This pull request contains the ability to be able to change the author and add contributors to an entry. As this is a feature that may not be needed by everyone you can activate it by defining a global variable in your wp-config.php:

define('LIVEBLOG_ALLOW_AUTHOR_SELECT', true);

When this feature is enabled the editor now has two drop downs below it allowing you to select and edit the authors and contributors. The author updates the actual author of the comment and the contributors are stored as comment meta as an array of ids.

Copy link
Contributor

@philipjohn philipjohn left a comment

Choose a reason for hiding this comment

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

A couple of minor pieces of feedback for you.

Regards LIVEBLOG_ALLOW_AUTHOR_SELECT I'd rather we just enabled it by default. It's not a feature I think we need to make people jump through a hoop for.

Instead of showing it by default, perhaps we introduce a menu of sorts (e.g. perhaps using the 3-dots style menu selector) and have the author and contributor options within there?

@@ -11,7 +11,13 @@ class WPCOM_Liveblog_Entry {
* @var string In case the current entry is an edit (replaces) of
* another entry, we store the other entry's ID in this meta key.
*/
const replaces_meta_key = 'liveblog_replaces';
const replaces_meta_key = 'liveblog_replaces';
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but can we fix the spacing here?

liveblog.php Outdated
* Get current user
*/
public static function get_current_user() {
if (!self::is_liveblog_editable()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do with updating the spacing here.

liveblog.php Outdated

return array(
'id' => $user->ID,
'key' => strtolower($user->user_nicename),
Copy link
Contributor

Choose a reason for hiding this comment

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

Spacing :)

getUsers(text, callback) {
const { config } = this.props;
getAuthors(text, config)
.timeout(10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this 10,000 seconds? Seems a bit much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Milliseconds, so its 10 seconds :)

Copy link
Contributor

Choose a reason for hiding this comment

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

wipes brow

@liamdefty
Copy link
Contributor Author

@philipjohn That sounds like a good idea 👍 We'll see what we can come up with.

@philipjohn
Copy link
Contributor

I feel like the UI could be prettier, but it does the job. If we end up with a need for more options in future, we can move to a more discrete UI element.

One issue - when selecting a user as both the author and a contributor, they show up twice in the entry.

fireshot capture 4 - liveblog wordpress dev - https___localhost_2018_01_24_liveblog_

Copy link
Contributor

@philipjohn philipjohn left a comment

Choose a reason for hiding this comment

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

Code looks good, just that one issue I noticed.

@philipjohn philipjohn added this to the 1.8 milestone Jan 27, 2018
@philipjohn philipjohn changed the base branch from master to 1.8-beta February 7, 2018 10:11
@philipjohn philipjohn merged commit 96a3879 into Automattic:1.8-beta Feb 15, 2018
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.

3 participants