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

Turn viewer and viewport service into core services. #1236

Merged
merged 1 commit into from
Dec 23, 2015

Conversation

cramforce
Copy link
Member

Credit goes to @aaron on whose PR this is based. I realized, though, that viewer and viewport would be pretty hard and frustrating, so I chose to finish it myself.

Good impact on code size for extensions that pull these in.

Part of #1055
Obsoletes #1122 (I'm so sorry, but looking at this change it was probably better this way).

Credit goes to @aaron on whose PR this is based. I realized, though, that viewer and viewport would be pretty hard and frustrating, so I chose to finish it myself.

Part of ampproject#1055
cramforce added a commit that referenced this pull request Dec 23, 2015
Turn viewer and viewport service into core services.
@cramforce cramforce merged commit 89222a8 into ampproject:master Dec 23, 2015
@cramforce cramforce deleted the viewer-service branch December 23, 2015 06:14
'src/service/history-impl.js',
'src/service/viewer-impl.js',
'src/service/viewport-impl.js',
'test/functional/test-cid.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @erwinmombay is there a way for us to declare whitelists for the whole test/ directory? I don't quite see a reason to be protective over this kind of uses in tests.

Copy link
Member

Choose a reason for hiding this comment

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

not currently, i didnt since i thought that people might unintentionally whitelist something they shouldnt have. but now that the list is growing, might as well.

added PR/feature here #1240

/cc @cramforce

@dvoytenko
Copy link
Contributor

LGTM

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.

3 participants