Skip to content

Removed ember-application-visit feature guards#13100

Merged
chancancode merged 1 commit intoemberjs:masterfrom
delftswa2016:feature-enabled
Mar 19, 2016
Merged

Removed ember-application-visit feature guards#13100
chancancode merged 1 commit intoemberjs:masterfrom
delftswa2016:feature-enabled

Conversation

@shashwat91
Copy link
Contributor

All the instances and feature guards of 'ember-application-visit' are removed as stated in issue #13088.

We are group of developers working started working on ember.js and we wanted to contribute something to the project. Thanks @chancancode for marking his issue as 'good for new contributors'.

@homu
Copy link
Contributor

homu commented Mar 13, 2016

☔ The latest upstream changes (presumably #13096) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

This should be removed

@chancancode
Copy link
Member

Left some line comments. After resolving them, can you squash your commits and rebase against master? Thank you!

@andrewflash andrewflash force-pushed the feature-enabled branch 4 times, most recently from 65b7d99 to a0d91b8 Compare March 13, 2016 10:43
@andrewflash
Copy link

@chancancode, you said that we can just merge this into the actual class definition instead of reopening the class. How can I do that? and I still get some errors in Travis CI. Any solution? Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

This should not be removed

Choose a reason for hiding this comment

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

I got an error when I keep that line, application is declared but never used.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@chancancode
Copy link
Member

you said that we can just merge this into the actual class definition instead of reopening the class. How can I do that?

Instead of..

const Application = Engine.extend({

  init: ...,

  ...

});

...

Application.reopen({

  visit: ...

});

...

It could just be...

const Application = Engine.extend({

  init: ...,

  ...,

  visit: ...

});

...

...

@shashwat91
Copy link
Contributor Author

@chancancode thanks for feedback. We are now working on it.

@chancancode
Copy link
Member

ApplicationInstance.reopen can probably be merged too

@sambit2
Copy link
Contributor

sambit2 commented Mar 13, 2016

@chancancode Thank you for your help as we are new into this. @andrewflash Good job

chancancode added a commit that referenced this pull request Mar 19, 2016
Removed `ember-application-visit` feature guards
@chancancode chancancode merged commit 94964e1 into emberjs:master Mar 19, 2016
@chancancode
Copy link
Member

Thanks! 👍

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.

5 participants