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

🏗 Added support for ESM and SxG environment in coverage-map #29800

Merged
merged 11 commits into from Sep 4, 2020

Conversation

xiexr151e
Copy link
Contributor

This PR incorporates a transformer that injects AMP's CSS styles into an HTML, if running under the SxG or ESM environment.
It is assumed that the input HTML will reside in examples, and then the transformed HTML will reside in dist/transformed.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

I've added a few minor comments, but this overall looks good. I've added @rcebulko for review / approval since this has to do with coverage.

build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
@rsimha rsimha requested a review from rcebulko August 12, 2020 16:00
@xiexr151e
Copy link
Contributor Author

Hi @rsimha @rcebulko, if you guys have the time, could you take a look at this PR again? Thank you guys so much for your correspondence!

build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
build-system/tasks/coverage-map/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Looking good!

@xiexr151e
Copy link
Contributor Author

@ampproject/build-cop Just one more thing before I merge, because local/dist tests will inconsistently fail.

@jridgewell
Copy link
Contributor

It's a flakey test. I opened #30124 to deal with it.

@xiexr151e xiexr151e merged commit 63848d3 into ampproject:master Sep 4, 2020
@xiexr151e xiexr151e deleted the derek-coverage-2 branch September 4, 2020 03:18
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
…ject#29800)

* incorporate testing for esm and sxg

* suggestions

* more suggestions, mostly cyan

* added map option for explore

* correct for esm only

* small change

* replace

* several changes

Co-authored-by: Derek Tse <derektse@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants