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

Add amp accordeon to component mapping #165

Merged
merged 3 commits into from
Apr 18, 2017
Merged

Add amp accordeon to component mapping #165

merged 3 commits into from
Apr 18, 2017

Conversation

renzit
Copy link
Contributor

@renzit renzit commented Feb 9, 2017

When implement the ampp-sidebar we need amp-accordion and this is not working because of the js is not in the component mapping. This commit fix that

@SGudbrandsson
Copy link
Contributor

@renzit the builds are failing - can you include the correct output for the .out files?

@renzit
Copy link
Contributor Author

renzit commented Mar 7, 2017

i dont know exactly how to.
This is one of the two problems in travis:

1) AmpTest::testFiles with data set "tests/test-data/full-html/validator-amp-accordion.html" ('tests/test-data/full-html/val...n.html', true)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 ------------------------------
-No custom amp script includes required
+'amp-accordion', include path 'https://cdn.ampproject.org/v0/amp-accordion-0.1.js'

But in the .out of the amp-accordion i dont know what is the output wanted.

The other error is the same but for amp-sidebar. and this is a pull request that iv made some weeks ago and it failed the test but the PR was accepted, if you can help me @sigginet , i can fix the text for the two amp elements.

@SGudbrandsson
Copy link
Contributor

@renzit from the command line inside the root of the project, run php tests/utilities/amp-regen then push the changes.

@renzit
Copy link
Contributor Author

renzit commented Mar 14, 2017

Thank you @sigginet.
This Fix:
this pull request [165],
and #163

where is documented this: php tests/utilities/amp-regen ?

@SGudbrandsson
Copy link
Contributor

@renzit right now it isn't documented (to my knowledge).
I just found this by reading all the source code

@SGudbrandsson SGudbrandsson merged commit ea60f9c into Lullabot:master Apr 18, 2017
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

2 participants