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 #1146

Merged
merged 26 commits into from
May 23, 2018
Merged

[Gutenberg] Add amp-timeago block #1146

merged 26 commits into from
May 23, 2018

Conversation

miina
Copy link
Contributor

@miina miina commented May 14, 2018

Aims to add Gutenberg blocks for the following components:

  • amp-timeago
  • amp-viz-vega
  • amp-fit-text

Edit:
Looks like amp-viz-vega is still experimental and amp-fit-text might make more sense implementing as part of extending core blocks, thus this PR will only cover adding amp-timeago.

Todo:

  • Configuration for compiling React / adding necessary libs, etc.
  • amp-timeago

},
dateTime: {
type: 'string'
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we also add Layout and width and height attributes?

@miina miina changed the title [WIP] [Gutenberg] Add AMP presentation blocks [Gutenberg] Add amp-timeago block May 16, 2018
@westonruter westonruter self-assigned this May 19, 2018
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.

I think we should .gitignore the complied JS files like assets/js/amp-blocks-compiled.js and instead make compiling them part of the the logic behind npm run build.

"scripts": {
"build": "grunt build",
"deploy": "grunt deploy"
"build": "cross-env BABEL_ENV=default webpack; grunt build",
Copy link
Member

Choose a reason for hiding this comment

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

There's surely a better way do do this then I've done here. It probably needs BABEL_ENV=production for example, but also I wonder if grunt-webpack should be used so that it can be called automatically as part of the build task.

"env": {
"default": {
"plugins": ["transform-runtime"]
}
Copy link
Member

Choose a reason for hiding this comment

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

Should there also be a production env here with optimizations for compiling to that target vs development?

@westonruter
Copy link
Member

I have a concern about the utility of a standalone timeago block. To me it seems it would make more sense as an inline element rather than a block in itself. For example, as a block this doesn't feel right:

image

Instead, it feels more natural to me that this would be inserted as some kind of inline element like:

image

In this case, an inline shortcode would feel more natural than a block. I know that insertion of inline images has been something that Gutenberg has not specifically been targeting. What if there was a new toolbar option added for “time ago” which allowed you to insert an amp-timeago element inside a paragraph?

But then again, it seems amp-timeago doesn't really lend itself well to being used inline if it requires a fixed width and height. I'm having a hard time seeing the normal use cases for an amp-timeago block. 🤔

@westonruter
Copy link
Member

@pbakaus Thoughts on intended usage of amp-timeago in a post content context? ☝️

@miina
Copy link
Contributor Author

miina commented May 21, 2018

@westonruter On being a standalone block -- true that it could likely make more sense as an inline block.
On of the possible uses as a separate block could be within Columns block for example:
Editor:
screen shot 2018-05-21 at 7 56 23 am
Frontend:
screen shot 2018-05-21 at 7 56 46 am

However, in this case, too, inline shortcode could be used instead. Not sure in which case standalone block would make more sense compared to inline insertion, perhaps if the editor would like to display the time difference separately with set width and height. One option could be adding pre-text and after-text as additional optional attributes.

@pbakaus
Copy link

pbakaus commented May 21, 2018

I also think it might not be very useful as standalone block, and would work better as inline block.Are inline blocks not a thing in Gutenberg? Is short code the only solution? If so, short code might be the way to go.

In general, amp-timeago's primary use case is around cases where you're lacking server support to do the same. Rendering the relative time stamp on the server would be greatly recommended. But in the case where WP is heavily cached, this component might make sense.

@westonruter
Copy link
Member

@pbakaus 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.

@pbakaus
Copy link

pbakaus commented May 21, 2018

aaah, that block ;)

Yes, that makes more sense. Actually strange that we don't have an equivalent to inline or inline-block in AMP. I'll try and see if we can do something about it..

@westonruter westonruter merged commit 090a4f6 into develop May 23, 2018
@westonruter
Copy link
Member

@miina would you resurrect amp-timeago on this branch since it got removed in #1165 with 329d3f4?

@miina
Copy link
Contributor Author

miina commented May 23, 2018

@westonruter Restored the block on this branch, looks like had to create a new PR (#1168).

@westonruter westonruter added this to the v1.0 milestone Sep 24, 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.

None yet

3 participants