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

revert: @angular/elements PR #19469 #20152

Merged
merged 1 commit into from Nov 3, 2017

Conversation

Projects
None yet
7 participants
@IgorMinar
Member

IgorMinar commented Nov 3, 2017

This PR was merged without API docs and general rollout plan.

We can't release this as is in 5.1 without a plan for documentation, cli integration, etc.

@googlebot googlebot added the cla: yes label Nov 3, 2017

@IgorMinar IgorMinar requested review from mhevery and gkalpak Nov 3, 2017

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Nov 3, 2017

Member

@gkalpak @mhevery sorry but we have to revert the elements PR. Without a plan how to bring this to production, this PR will only cause issues as we get close to the 5.1 release and the necessary prerequisites are note ready.

Please propose a full rollout plan that includes docs, cli integration, google3 integration - see the design doc template for all the requirements. We should also have a clear owner of this effort who could work with @manughub on rolling this out. This feature was originally planned for rollout in Q1 of next year, so unless we changed our mind about this, we should have a discussion about it.

Member

IgorMinar commented Nov 3, 2017

@gkalpak @mhevery sorry but we have to revert the elements PR. Without a plan how to bring this to production, this PR will only cause issues as we get close to the 5.1 release and the necessary prerequisites are note ready.

Please propose a full rollout plan that includes docs, cli integration, google3 integration - see the design doc template for all the requirements. We should also have a clear owner of this effort who could work with @manughub on rolling this out. This feature was originally planned for rollout in Q1 of next year, so unless we changed our mind about this, we should have a discussion about it.

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Nov 3, 2017

@trotyl

This comment has been minimized.

Show comment
Hide comment
@trotyl

trotyl Nov 3, 2017

Contributor

Could we just have one commit for the elements-builds repo first, so we can use that for demos or other explore works instead of all building our own 🙏

Sorry, found that already exists 😄

Contributor

trotyl commented Nov 3, 2017

Could we just have one commit for the elements-builds repo first, so we can use that for demos or other explore works instead of all building our own 🙏

Sorry, found that already exists 😄

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Nov 3, 2017

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Nov 3, 2017

Member
Member

IgorMinar commented Nov 3, 2017

@IgorMinar IgorMinar added this to the 5.0.x milestone Nov 3, 2017

@gkalpak

gkalpak approved these changes Nov 3, 2017

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Nov 3, 2017

Member

@IgorMinar, is it intentional that this PR does not reverts the whole original PR? All packages/elements/* files are still left behind.

Member

gkalpak commented Nov 3, 2017

@IgorMinar, is it intentional that this PR does not reverts the whole original PR? All packages/elements/* files are still left behind.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak
Member

gkalpak commented Nov 3, 2017

@gkalpak

The PR was not properly reverted.

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Nov 3, 2017

Member
Member

IgorMinar commented Nov 3, 2017

revert: feat(elements): implement `@angular/elements` #19469
This PR was merged without API docs and general rollout plan.

We can't release this as is in 5.1 without a plan for documentation, cli integration, etc.
@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar
Member

IgorMinar commented Nov 3, 2017

@gkalpak done

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Nov 3, 2017

Member

lint check is failing because the revert removed support for "element" commit message scope. I think we should just ignore this as it won't affect anything after the revert is merged.

Member

IgorMinar commented Nov 3, 2017

lint check is failing because the revert removed support for "element" commit message scope. I think we should just ignore this as it won't affect anything after the revert is merged.

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Nov 3, 2017

@gkalpak

gkalpak approved these changes Nov 3, 2017

😢

@mhevery

mhevery approved these changes Nov 3, 2017

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Nov 3, 2017

Member

sending this for merge. for explanation of the lint failure see: #20152 (comment)

the travis-ci/push failure is irrelevant - I accidentally pushed an older version of this branch to the upstream repo - this branch has since been deleted, but the ci failure didn't disappear from this pr.

Member

IgorMinar commented Nov 3, 2017

sending this for merge. for explanation of the lint failure see: #20152 (comment)

the travis-ci/push failure is irrelevant - I accidentally pushed an older version of this branch to the upstream repo - this branch has since been deleted, but the ci failure didn't disappear from this pr.

@vicb vicb merged commit 3997d97 into angular:master Nov 3, 2017

4 of 6 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
ci/circleci: lint Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
code-review/pullapprove Approved by all reviewer groups.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment