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

[WIP] Add Title block for AMP Stories #2072

Closed
wants to merge 2 commits into from

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Apr 5, 2019

  • Begins the work to add a title block
  • Some improvements are still needed, like possibly making this a dynamic block, and implementing the layout controls

title-block

Closes #2007

kienstra added 2 commits April 5, 2019 05:03
This has alignment and controls to select h2, h3, or h4.
The h2, h3, and h4 controls don't work yet,
and other improvements are needed.
This seems to only apply
to the Header block.
@googlebot googlebot added the cla: yes Signed the Google CLA label Apr 5, 2019
@kienstra kienstra changed the title Add Title block for AMP Stories [WIP] Add Title block for AMP Stories Apr 5, 2019
export const name = 'amp/amp-story-title';

export const settings = {
title: __( 'AMP Title', 'amp' ),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
title: __( 'AMP Title', 'amp' ),
title: __( 'Story Title', 'amp' ),

Copy link
Member

Choose a reason for hiding this comment

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

Don't accept this suggestion unless you want the CLA bot to start ❌'ing your PR 😏

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Importantly, modifying the title of the post needs to automatically update the block's text content. And modifying the text content, needs to modify the post title.

@@ -10,6 +10,7 @@ const Shortcuts = ( { insertBlock } ) => {
const blocks = [
'amp/amp-story-text',
'core/image',
'amp/amp-story-title',
Copy link
Member

Choose a reason for hiding this comment

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

Is this right to include as a shortcut? It would only be used on the first page, and it should be part of the post type template so that it is inserted automatically when you create a new story.

/**
* Saves the results of the edit component.
*
* @todo: possibly use PHP to render this instead, as the title could change.
Copy link
Member

Choose a reason for hiding this comment

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

Correct. The return value should be null. A render_callback should be used to render the current title with wptexturize and all the normal the_title behaviors.

@swissspidy
Copy link
Collaborator

@kienstra What's the current status here?

Asking because I was looking at consolidating some code in the other blocks and I think I can use that to easily create these meta blocks here as well.

@miina
Copy link
Contributor

miina commented Apr 8, 2019

@kienstra Also see this (in case you didn't see the discussion here): #2007 (comment)

@kienstra
Copy link
Contributor Author

kienstra commented Apr 8, 2019

Status

Hi @swissspidy,
Thanks for checking, there aren't any commits for this PR that aren't pushed. So feel free to take this over if you'd like, or apply this PR's requirements in another PR.

@miina
Copy link
Contributor

miina commented Apr 9, 2019

Reassigning the ticket to @swissspidy based on the conversation. As far as I understood then @kienstra moved over to working on other tickets already.

Let me know if this information is incorrect.

@swissspidy
Copy link
Collaborator

Closing this in favor of #2097.

@swissspidy swissspidy closed this Apr 9, 2019
@kienstra
Copy link
Contributor Author

kienstra commented Apr 9, 2019

Sounds good, @miina. Thanks for handling this, @swissspidy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants