Refactor AMP class and fix diffing #72
Assignees
Labels
Comments
Merged
|
The code refactoring has been done and issues with diff output have been fixed. Here is an example of diff output before. $ ./amp-console amp:convert sample-html/several_errors.html --full-document --diff --no-orig-and-warn
--- Original
+++ New
@@ @@
This example has several problems - see the .out file.
---><html><head><meta charset="pick-a-key"><link rel="canonical" href="./regular-html-version.html"><style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript><script type="javascript">
- // Let's go crazy. Uhm, no, hahaha.
- </script></head><body>
- <table><tr><td>Tables are allowed</td></tr></table><amp-img src="dimensions_are_missing.jpg" layout="fixed"></amp-img><!-- We don't allow percent as a size unit either. --><amp-ad width="100%" height="300"><!-- <script>document.write(�)</script> --></amp-ad><amp-ad width="42" height="42" made_up_attribute="oh hi"></amp-ad></body></html>
+--><html amp><head>
+ <meta>
+ <link rel="canonical" href="./regular-html-version.html">
+ <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+
+</head>
+<body>
+ <table><tr><td>Tables are allowed</td></tr></table>
+ <amp-img src="dimensions_are_missing.jpg" layout="fixed"></amp-img>
+ <!-- We don't allow percent as a size unit either. -->
+ <amp-ad width="100%" height="300">
+ <!-- <script>document.write(…)</script> -->
+ </amp-ad>
+ <amp-ad width="42" height="42"></amp-ad>
+</body>
+
+</html>This is the diff output now. Notice how streamlined and useful it is. The previous output was quite useless due to formatting issues (which increased the diff shown when the "actual" diff was much less) $ ./amp-console amp:convert sample-html/several_errors.html --full-document --diff --no-orig-and-warn
--- Original
+++ New
@@ @@
This example has several problems - see the .out file.
---><html><head>
- <meta charset="pick-a-key">
+--><html amp><head>
+ <meta>
<link rel="canonical" href="./regular-html-version.html">
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
- <script type="javascript">
- // Let's go crazy. Uhm, no, hahaha.
- </script>
+
@@ @@
</amp-ad>
- <amp-ad width="42" height="42" made_up_attribute="oh hi"></amp-ad>
+ <amp-ad width="42" height="42"></amp-ad>
</body>
</html>Code was merged to master. Closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The AMP class is getting unwieldy. There is a lot of scope for refactoring code into smaller functions. Do this. Additionally, we have a facility to be able to
diffthe initial HTML input with the AMPized output HTML. This feature has some issues -- address them.The text was updated successfully, but these errors were encountered: