Add filtering for AMP examples #2494
Conversation
|
Moved |
|
Thanks - changes are live @0.2.15 |
| @@ -657,7 +662,7 @@ class SamplesBuilder { | |||
| '$localization': { | |||
| 'path': `/{locale}${this._getPreviewRoute(sample)}`, | |||
| }, | |||
| 'formats': [this._getSampleFormat(parsedSample)], | |||
| 'formats': this._getSampleFormats(parsedSample), | |||
There was a problem hiding this comment.
This is now redundant with filters. Can we somehow merge this with filters? //cc @matthiasrohmer
There was a problem hiding this comment.
Not sure I follow? Filters (@filter) are section-level and formats are article-level. This makes it aligned (more or less) with non-example articles which have both.
While we could possibly guess the formats in a smarter way, I'd prefer the less-eager model where the example author is explicit in which format the example is useful for. E.g. just because an example works in emails doesn't make it a good example of something to use in an email.
There was a problem hiding this comment.
You're right, forgot that filters are on section level. We could even go as far as validating section filter against formats defined in the front matter.
There was a problem hiding this comment.
I think that makes sense.
Looking at the code, seems we have two options:
-
Move the call to
_getSampleFormatsinto_parseSample, e.g.parsedSample.formats = this._getSampleFormats(parsedSample)and then verify when traversing the sections in_parseSample. -
Move the whole
_getSampleFormatslogin into ampproject/amp-by-example.
Any preference?
There was a problem hiding this comment.
I think 2. would be good, this avoids also that the method is being called twice in this file.
There was a problem hiding this comment.
@fstanis good with merging this branch?
filters:frontmatter@formatin the comments (context)