-
Notifications
You must be signed in to change notification settings - Fork 25
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
New Transformer: AutoExtensions #32
Comments
I've started working on this. I already did that for my CMS and I am trying to bring that into the I noticed there are AMP extensions that define additional elements (e.g. Resulting in a list like that: const AMP_EXTENSIONS_MAP = [
// amp-ad
"amp-embed" => "amp-ad",
// amp-bind
"amp-state" => "amp-bind",
"amp-bind-macro" => "amp-bind",
// amp-embedly-card
"amp-embedly-key" => "amp-embedly-card",
// amp-google-assistant-assistjs
"amp-google-assistant-assistjs-config" => "amp-google-assistant-assistjs",
"amp-google-assistant-voice-button" => "amp-google-assistant-assistjs",
"amp-google-assistant-voice-bar" => "amp-google-assistant-assistjs",
"amp-google-assistant-inline-suggestion-bar" => "amp-google-assistant-assistjs"
// ...
]; The generated PHP file would be part of the release (i.e. the repository) and can be updated via a NPM command. Would that also be a sufficient solution here? If so, should I generate a PHP file (maybe even use that to generate the |
You're on the right track, but I think most of what you're suggesting is already mostly complete in #100. The extension dependencies are encoded in the validator spec, and once that spec is available in PHP form it can be used to do the auto-extension insertion. So you can see in amp-toolbox-php/src/Validator/Spec/Tag/AmpState.php Lines 20 to 25 in c8042c0
And it specifies that it requires the amp-toolbox-php/src/Validator/Spec/Tag/AmpState.php Lines 68 to 70 in c8042c0
Once that is merged, creating the |
That looks promising, thanks. I'll take a look at it and wait for the merge then. :) |
Hey @saitho, I already have a PR in the works for the auto-extensions as well. You can currently see it in the https://github.com/ampproject/amp-toolbox-php/tree/add/32-auto-extensions branch right now. It was just blocked because the validator spec is indeed needed to make this work across all edge cases. So, before you put any work into this, please wait for me to push everything into its final state. I'll ping you on the new PR for the auto-extensions so we can exchange approaches and discuss implementation details. |
Sorry, the link above was not very helpful, here is a better one that shows the current changes for this new auto-extensions transformer: add/amp-validator-spec...add/32-auto-extensions |
Sure, thanks! I've already implemented my approach on my end outside of the amp-toolbox. Will try your branch when it's ready. :) |
I've turned the branch into a draft PR for a better overview: #210 |
The text was updated successfully, but these errors were encountered: