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

Editor: Replace the default auto-draft title of new Gutenberg posts. #27297

Merged
merged 6 commits into from
Sep 20, 2018

Conversation

kwight
Copy link
Contributor

@kwight kwight commented Sep 18, 2018

If we receive an auto-draft on opening the Gutenberg editor, replace its default "Auto Draft" title with that of the wp-admin: "Add title". This should also allow easily adding the demo content when required as a follow-up PR.

screen shot 2018-09-19 at 12 17 20 pm

Fixes #27162 .

Testing

  1. Load this branch, go to https://calypso.localhost:3000/gutenberg/post, and select a site.
  2. This will create a new Gutenberg post, which should have "Add title" for the title.

@matticbot
Copy link
Contributor

@kwight kwight requested a review from a team September 18, 2018 23:18
@kwight kwight self-assigned this Sep 18, 2018
@kwight kwight added [Feature] Post/Page Editor The editor for editing posts and pages. [Type] Task [Goal] Gutenberg Working towards full integration with Gutenberg [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 18, 2018
@@ -53,13 +53,27 @@ class GutenbergEditor extends Component {
}
}

const addGutenbergDemoContent = post => {
const title = {
raw: 'Welcome to the Gutenberg Editor',
Copy link
Contributor

Choose a reason for hiding this comment

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

We likely need to localize this I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Automattic/calypso-sdk @Automattic/i18n any objections if we do the following?

import { translate } from 'i18n-calypso';
//...
const addGutenbergDemoContent = post => {
	const title = {
		raw: translate( 'Welcome to the Gutenberg Editor' ),
		rendered: translate( 'Welcome to the Gutenberg Editor' ),
	};

	return {
		...post,
		title,
	};
};

Copy link
Member

Choose a reason for hiding this comment

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

I've tested something like the following:

import { translate } from 'i18n-calypso';
//...
const demoTitleText = translate( 'Welcome to the Gutenberg Editor' );
const title = {
    raw: demoTitleText,
    rendered: demoTitleText,
};

Where a translation exists, it will render correctly.

In this instance we have translation, but you won't see translated output yet on /gutenberg/post/{site}.

I've added the source path clients/gutenberg/editor/ to our views, so it should be picked up soon. I'll keep an 👁 on it. 👍

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this will always live in Calypso, so I don't see any downsides of using Calypso's i18n.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I meant to double-check it was translated at output, but no, it's not (which certainly makes sense). Will update.

}

return null;
const sitePostData = get( requestSitePost( siteId, postId ), 'data', null );
return sitePostData && 'auto-draft' === sitePostData.status
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also wondering why an auto-draft is a requirement for Gutenberg init, but we can explore this in a core PR enhancement.

@gwwar gwwar requested a review from aduth September 19, 2018 14:10
@Copons
Copy link
Contributor

Copons commented Sep 19, 2018

This tests as expected, but I'm unclear of the why. 🤔

Why would we want to force a title into the editor when the post has an auto-draft status?

Are there any chances that auto-draft is set at some point to the post (my understanding is that it's only used for revisions, but still), effectively replacing the post title with the "Welcome" message?

@@ -53,13 +53,27 @@ class GutenbergEditor extends Component {
}
}

const addGutenbergDemoContent = post => {
const title = {
raw: 'Welcome to the Gutenberg Editor',
Copy link
Member

@simison simison Sep 19, 2018

Choose a reason for hiding this comment

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

Is it going to be clear to users at this point what "Gutenberg" brand is? Should we instead roll it out as:

Welcome to the new editor

...PR might not be the right place for this discussion, though! Good to check on wording, is what I'm saying. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

One other thing, could we maybe decorate content using a query param like ?demo to start? Without this, I think we'll want to blank out the title set here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it going to be clear to users at this point what "Gutenberg" brand is?

I do see value in it being the same as wp-admin, particularly if users find themselves back and forth for any reason. But yes, definitely a valid bigger question for the core project 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

For context I meant to be the same as wp-admin as in this new post screenshot, not the demo preview. (We can tackle that in another follow up with some trigger like a query param.)

screen shot 2018-09-19 at 10 41 41 am

@kwight
Copy link
Contributor Author

kwight commented Sep 19, 2018

Why would we want to force a title into the editor when the post has an auto-draft status?

To load, Gutenberg requires a valid post (p7jreA-1N0-p2), so if we're creating a new post, that comes in the form of an auto-draft (and auto-drafts have a default title of "Auto Draft"). Something more useful to the user would be better (and this PR lines that up with the default content for the Gutenberg demo in wp-admin).

Are there any chances that auto-draft is set at some point to the post (my understanding is that it's only used for revisions, but still), effectively replacing the post title with the "Welcome" message?

Not sure what you mean, but the first time the auto-draft is, erm, auto-saved, its status will go from auto-draft to a normal draft (I've confirmed this behaviour in both core and Calypso). Auto-drafts are cleaned up by core every seven days.


return {
...post,
title,
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't tested it, but I have a feeling that this won't work, notably at the point that we actually save. Since what we're feeding here is instructing Gutenberg to what the saved value of the post is, Gutenberg is not aware that the title has been modified and thus will not include it in the save payload. Since it won't be included in the save payload, it won't be updated server-side and thus the existing title (presumably "Auto Draft") will remain.

This is what the overridePost prop of the Editor component (and argument of initializeEditor) is meant to represent:

https://github.com/WordPress/gutenberg/blob/f175813c4871dd6472e33fd528bae79b6d9a24d0/edit-post/index.js#L70

This is also subject to refactoring in WordPress/gutenberg#9403, both in form of renaming this to initialEdits and, in doing so, not expecting values to be passed in shape of object with raw / rendered.

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 did verify that it saves properly, but I'm not sure if that's a good or bad thing to what you expected 🙃

I didn't know about overridePost though, so I'll dig into that too as an option (even if we need to tighten up after a refactor). Thanks!

@gwwar
Copy link
Contributor

gwwar commented Sep 19, 2018

See also https://github.com/WordPress/gutenberg/blob/45bc8e4991d408bca8e87cba868e0872f742230b/lib/client-assets.php#L1346 where it looks like we probably eventually pass this encoded json to the overridePost field on init.

@kwight kwight changed the title Editor: Add the Gutenberg demo title to new Gutenberg posts. Editor: Replace the default auto-draft title of new Gutenberg posts. Sep 19, 2018
@kwight
Copy link
Contributor Author

kwight commented Sep 19, 2018

5954c0e rolls this back to ignore the situation of demo content, and just gives us a corrected title of "Add title" when loading a new Gutenberg post. A follow-up PR can handle injecting demo content (maybe given a query param) on a user's first view, say from the opt-in sidebar visual from #26592.

@Copons
Copy link
Contributor

Copons commented Sep 20, 2018

@kwight thanks for the explanation!

Another question: what if we passed overridePost={ { title: '' } } instead?
I tried it and it seems to work as I'd expect: the title begins with the "Add title" placeholder, and it doesn't change at first auto-save.

@kwight
Copy link
Contributor Author

kwight commented Sep 20, 2018

@Copons exactly what @aduth pointed to, and it's the most recent commit :)

@kwight
Copy link
Contributor Author

kwight commented Sep 20, 2018

@Copons Oh! I get what you mean – I've been ignoring the placeholder aspect of it, sorry was confused 🙃

client/gutenberg/editor/main.jsx Outdated Show resolved Hide resolved
bodyPlaceholder: translate( 'Write your story' ),
};

const isAutoDraft = () => post && 'auto-draft' === post.status;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note, but we can also move this logic to mapStateToProps:

const mapStateToProps = ( state, { siteId, postId, uniqueDraftKey } ) => {
	const draftPostId = get( getHttpData( uniqueDraftKey ), 'data.ID', null );
	const post = getPost( siteId, postId || draftPostId );
    const isAutodraft = get( post, 'status', null );
    const overridePost = isAutodraft ? { title: '' } : null;

	return {
		siteSlug: getSiteSlug( state, siteId ),
		post,
        overridePost,
	};
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, I really was looking at this too much 🙃 Yes, thanks!

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Thanks @kwight ✨ This works great. I left an optional note, but free free to :shipit:

@kwight kwight merged commit c5d73bf into master Sep 20, 2018
@kwight kwight deleted the update/gutenberg-demo-title branch September 20, 2018 19:53
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. [Goal] Gutenberg Working towards full integration with Gutenberg [Type] Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants