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

[Gutenberg] Add amp-timeago block #1168

Merged
merged 18 commits into from
May 30, 2018
Merged

[Gutenberg] Add amp-timeago block #1168

merged 18 commits into from
May 30, 2018

Conversation

miina
Copy link
Contributor

@miina miina commented May 23, 2018

Adds amp-timeago Gutenberg block.

Note: see discussion in #1146 on the usefulness of having this as a standalone block.

@miina miina mentioned this pull request May 23, 2018
2 tasks
@westonruter
Copy link
Member

To quote #1146 (comment):

It seems difficult to use amp-timeago in an a non-block context, though. Here I'm just talking about block and inline elements, not specifically Gutenberg blocks. For example:

<p>The new year started <amp-timeago width="auto" height="20" datetime="2018-01-01T00:00:00.000Z">January 1st, 2108</amp-timeago> and I am so glad.</p>

Comes out:

image

And:

<p>The new year started <amp-timeago width="100" height="20" datetime="2018-01-01T00:00:00.000Z">January 1st, 2108</amp-timeago> and I am so glad.</p>

Comes out:

image

Both of these clearly are not right in an inline context where flowing text is desired. I'm sure it has to do with AMP's need for layout as for why the element has these constraints which make it difficult for use in an inline context. I've noticed a similar issue with amp-mathml, where I'd normally want Math to appear inline, but this again is a challenge for layout.

If these constraints remain, I'm not sure that amp-timeago really makes sense as a Gutenberg block or a shortcode. It would make more sense in the theme templates like for the post's publish time, where it does not exist in the context of flowing text.


attributes: {
align: {
type: 'string'
Copy link
Member

Choose a reason for hiding this comment

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

I think the attributes should be sourced from the children element attributes, as done in amp-fit-text: c3d9915

This prevents the data from being duplicated between the block comment attributes and the content of the block. This means that if you edit the HTML you don't have to make sure that the two sets of attributes are in sync.

Copy link
Member

Choose a reason for hiding this comment

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

Please do the same for amp-mathml and any other blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added the sources, currently within this PR, however, not sure if this PR will get merged anytime soon, let me know if it would be better to add in a separate 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.

@miina I think we might as well merge amp-timeago as an experimental block for testing. So I think we should go ahead and merge the PR. Nevertheless, one thing is blocking this. I'm getting an error:

Jed localization error:

Error: Domain amp was not found.

I have both languages/amp.pot and languages/amp-js.pot files in the filesystem.

Also, it seems JS source maps aren't being when compiling? It's making it hard to debug. Can that be enabled?

@westonruter
Copy link
Member

@miina Actually, the error fixed in f65439a and the localization error are both in develop too. So this PR is a good place to fix them.

@miina
Copy link
Contributor Author

miina commented May 30, 2018

@westonruter Looks like this error occurs if amp domain is used in JS files before locale data is set (wp.i18n.setLocaleData), this was the case for block files (/blocks/*) -- locale data was set after the script. Making sure that it's done before fixes the error. (Should we fixed now.)

@westonruter westonruter merged commit 7918b41 into develop May 30, 2018
@westonruter westonruter deleted the add/prenetation_blocks branch May 30, 2018 21:09
@westonruter westonruter added this to the v1.0 milestone May 30, 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.

2 participants