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

PostTitle: be able to set post title via own prop #20609

Closed
wants to merge 3 commits into from

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Mar 3, 2020

Description

This PR proposes to be able to set the title prop of the <PostTitle /> explicitly., giving priority to the post title attribute, trying to guarantee it won't affect the current and natural behavior of the component.

Particularly, we are working on some stuff related to the page preview, and we'd like to use <PostTitle /> component on this context, where among many particularities, the site post doesn't exist. With the current implementation, is not possible to set arbitrarily the title.

How has this been tested?

  • Start to create a new post. It should the placeholder text, as usual.

image

  • Edit a post. You should see the post title, as usual.

image

  • Try to play with this component passing the <PostTitle /> as property. Just for testing purposes, I've edited the component with something like this:
//....
<ObserveTyping>
	<CopyHandler>
		<PostTitle fallbackTitle="Static Title Value!" />
		<BlockList />
	</CopyHandler>
</ObserveTyping>
//....

image

The static title value should be shown only if the post doesn't have defined a title. Once the post gets its title value, it should be shown properly.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.

@retrofox retrofox added the [Type] Enhancement A suggestion for improvement. label Mar 3, 2020
@retrofox retrofox requested review from getdave, jeryj and aduth and removed request for aduth March 3, 2020 13:16
@github-actions
Copy link

github-actions bot commented Mar 3, 2020

Size Change: -66 B (0%)

Total Size: 865 kB

Filename Size Change
build/block-editor/style-rtl.css 10.5 kB +1 B
build/block-library/editor-rtl.css 7.36 kB -41 B (0%)
build/block-library/editor.css 7.36 kB -41 B (0%)
build/block-library/index.js 116 kB -5 B (0%)
build/block-library/style-rtl.css 7.5 kB -3 B (0%)
build/block-library/style.css 7.51 kB -3 B (0%)
build/blocks/index.js 57.6 kB -10 B (0%)
build/edit-post/index.js 90.9 kB +19 B (0%)
build/editor/index.js 44.6 kB +17 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style.css 10.5 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.6 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/style-rtl.css 8.53 kB 0 B
build/edit-post/style.css 8.53 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@retrofox retrofox requested a review from aduth March 3, 2020 14:11
retrofox added a commit to Automattic/wp-calypso that referenced this pull request Mar 3, 2020
It implements a hacky solution to propagate the template title to the <PostTitle /> component, since it isn't possible to do it in thr current implementation of the component. I've created a PR, but in the meanwhile we can take this patch in order to give a quick solution. We can iterate later once the code PR is ship? WordPress/gutenberg#20609
@@ -141,7 +141,7 @@ const applyWithSelect = withSelect( ( select ) => {

return {
isCleanNewPost: isCleanNewPost(),
title: getEditedPostAttribute( 'title' ),
title: getEditedPostAttribute( 'title' ) || ownTitle,
Copy link
Contributor

Choose a reason for hiding this comment

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

With this priority, you will never be able to use the override on a page that already has a title assigned and getEditedPostAttribute will be returning value. It will only work on title-less page which I don't think is the goal 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.

I thought title will try to get the value from getEditedPostAttribute( 'title' ) as first option. If it isn't defined then it will try to apply the second part which is ownTitle. Testing again.

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 will only work on title-less page which I don't think is the goal here

Actually it's the desired goal 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.

do you think that the explicit value should be stronger than site post?

Copy link
Contributor

@getdave getdave Mar 4, 2020

Choose a reason for hiding this comment

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

I think it depends on what you are trying to achieve.

Is the custom title provided as a prop conceptually a "placeholder" value or not.

In my head the data flow model suggests that if you provide a prop such as title then you should expect that title to be reflected in the rendered component. To have a side effect suddenly change the title to something else entirely could feel very odd. I've provided a title so show that title.

Alternatively if you call the prop fallbackTitle then it might be legitimate to have the title rendered unless the post attribute title isn't available.

Copy link
Contributor Author

@retrofox retrofox Mar 4, 2020

Choose a reason for hiding this comment

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

What do you think about __experimentalFallbackTitle?

Copy link
Contributor

Choose a reason for hiding this comment

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

This way you can really only use the component override in an iframe (where there will never be a post title) but that's not how normal usages would look like. If we make it title and change the order in the condition, I think that would be a much more useful API for others too. That way you will be able to render strings styled as titles anywhere. Up to you to make the decision but I find it strange in the current form

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think makes sense what you do suggest. Addressed here.

retrofox added a commit to Automattic/wp-calypso that referenced this pull request Mar 4, 2020
* spt: set template using post-title component
It implements a hacky solution to propagate the template title to the <PostTitle /> component, since it isn't possible to do it in thr current implementation of the component. I've created a PR, but in the meanwhile we can take this patch in order to give a quick solution. We can iterate later once the code PR is ship? WordPress/gutenberg#20609

* spt: refact adding setTemplateTitle

* spt: apply delay for the first-rendering cycle

* spt: remove core/heading teplate title from blocks

* spt: add padding top to the template title

* spt: add missing template title classname

* Update apps/full-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/components/block-iframe-preview.js

Improve jsdoc. Props to @getdave :-)

Co-Authored-By: Dave Smith <getdavemail@gmail.com>

* Update apps/full-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/components/block-iframe-preview.js

Improve jsdoc. Props to @getdave

Co-Authored-By: Dave Smith <getdavemail@gmail.com>

* Update apps/full-site-editing/full-site-editing-plugin/starter-page-templates/page-template-modal/styles/starter-page-templates-editor.scss

Set the CSS class name more targetted to the SPT scope.

Co-Authored-By: Dave Smith <getdavemail@gmail.com>

* spt: reduce redice hook effect requeriments

* spt: remove setTimeout hack

* spt: always render preview content

* spt: remove unneeded id attribute

* spt: removes CSS hack

* spt: improve doc

Co-authored-by: Dave Smith <getdavemail@gmail.com>
@aduth
Copy link
Member

aduth commented Mar 5, 2020

Particularly, we are working on some stuff related to the page preview, and we'd like to use <PostTitle /> component on this context, where among many particularities, the site post doesn't exist. With the current implementation, is not possible to set arbitrarily the title.

I'm not sure I follow this. In the context of @wordpress/editor components, there would always expect to be some post entity to work with, even if not saved yet (or auto-draft). It would be my expectation that to explicitly set a title, you would dispatch to update the draft changes, which would be reflected in the rendered component:

wp.data.dispatch( 'core/editor' ).editPost( { title: 'My explicit title' } );

It doesn't seem consistent to me in how editor components are intended to operate that we would need to bypass the post entity for showing a title.

@retrofox
Copy link
Contributor Author

retrofox commented Mar 5, 2020

I'm not sure I follow this. In the context of @wordpress/editor components, there would always expect to be some post entity to work with, even if not saved yet (or auto-draft). It would be my expectation that to explicitly set a title, you would dispatch to update the draft changes, which would be reflected in the rendered component:

We want to make a preview of the post content, for instance. Take a look at the following picture:

image

There, there isn't a blog post at all. It's just a handful of blocks. But I'd like to put the post title among the page blocks, as part of the preview, too.

wp.data.dispatch( 'core/editor' ).editPost( { title: 'My explicit title' } );

There isn't a blog post to target.

It doesn't seem consistent to me in how editor components are intended to operate that we would need to bypass the post entity for showing a title.

Yeah, that is what I thought initially. It's fair enough.

@aduth
Copy link
Member

aduth commented Mar 5, 2020

What behaviors are you trying to reuse? Because all of the components in editor are pretty targeted at use with interoperating with a post, so they don't make much sense to use outside that context. But if there are specific behaviors of the component you'd like to leverage, maybe there's opportunity to extract a lower-level reusable component.

Also worth noting, in case it's unclear, that by "post", I consider to include pages as well, since a page is simply one of the possible types of a post.

I believe as part of the full-site editing effort, there may also be some work to extract a "Post Title" block (#18461), though even this I would still expect to interact with a post entity.

@retrofox
Copy link
Contributor Author

retrofox commented Mar 5, 2020

But if there are specific behaviors of the component you'd like to leverage, maybe there's opportunity to extract a lower-level reusable component.

Yeah, I think it fits very well with our case.

Also worth noting, in case it's unclear, that by "post", I consider to include pages as well, since a page is simply one of the possible types of a post.

Yeah, got it. I'm aware of this as well :-D

I believe as part of the full-site editing effort, there may also be some work to extract a "Post Title" block (#18461), though even this I would still expect to interact with a post entity.

Taking a look at this implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants