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

- Refactor ember-debug to use ES6. #387

Merged
merged 1 commit into from
Jun 22, 2015

Conversation

Mawaheb
Copy link
Contributor

@Mawaheb Mawaheb commented Jun 8, 2015

OK, I believe this is ready, But, there are a few problems that I faced:

  • I couldn't refactor the contents of /vendor, when I tried, I got an error "Can't use block-scoped let, const out of restricted mode."
  • Because arrow functions should always be anonymous, I couldn't/figure out how to refactor this one to use this instead of self
    similar problem with this one

As always, Any suggestions, comments, notes, things to be changed or feedback is always appreciated

@Mawaheb Mawaheb force-pushed the refactorEmberDebug branch from fb5588b to 8820c43 Compare June 8, 2015 15:55
@teddyzeenny
Copy link
Contributor

👍 Thanks @Mawaheb! I will review this weekend at the latest.

@Mawaheb
Copy link
Contributor Author

Mawaheb commented Jun 9, 2015

No problem :) My pleasure.

self.onConnectionReady();
const Ember = window.Ember;
const { $, A, computed, RSVP, Object } = Ember;
const { Promise } = RSVP;
Copy link
Contributor

Choose a reason for hiding this comment

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

const { Promise, resolve } = RSVP;

@Mawaheb Mawaheb force-pushed the refactorEmberDebug branch from 8820c43 to 7e87462 Compare June 11, 2015 10:53
@Mawaheb
Copy link
Contributor Author

Mawaheb commented Jun 11, 2015

Thanks for your feedback @patsy-issa :), I made the changes accordingly.

@patsy-issa
Copy link
Contributor

Np ^^ I ll take a look at the other files when I have the time.

RSVP.resolve(this.connect(), 'ember-inspector').then(function() {
self.onConnectionReady();
const Ember = window.Ember;
const { $, A, computed, RSVP, Object } = Ember;
Copy link
Contributor

Choose a reason for hiding this comment

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

Object has meaning in native javascript. You should rename it to something like EmberObject:
const { $, A, computed, RSVP, Object: EmberObject } = Ember;


var ViewDebug = Ember.Object.extend(PortMixin, {
const ViewDebug = Object.extend(PortMixin, {
Copy link
Contributor

Choose a reason for hiding this comment

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

export default

@teddyzeenny
Copy link
Contributor

@Mawaheb this is great!

I couldn't refactor the contents of /vendor, when I tried, I got an error "Can't use block-scoped let, const out of restricted mode."

Yeah vendor files are not transpiled. We can't (and shouldn't) refactor them.

Because arrow functions should always be anonymous, I couldn't/figure out how to refactor this one to use this instead of self.

You can declare them as variables instead of functions:

const pinView = () => {
};
Ember.deprecate = (message, test, options) => {
};

@Mawaheb Mawaheb force-pushed the refactorEmberDebug branch 2 times, most recently from fcb5db3 to 99a0277 Compare June 15, 2015 14:33
@Mawaheb
Copy link
Contributor Author

Mawaheb commented Jun 15, 2015

Thank you for your feedback, I modified the code accordingly.

One thing, When I pulled the latest master, and when I rebased, there were some merge/rebase conflicts, I hope I fixed them the right way.

@teddyzeenny
Copy link
Contributor

Sorry @Mawaheb this needs another rebase.

@Mawaheb Mawaheb force-pushed the refactorEmberDebug branch 2 times, most recently from c9afc0d to d768377 Compare June 17, 2015 15:44
@Mawaheb
Copy link
Contributor Author

Mawaheb commented Jun 17, 2015

Sorry for the inconvenience, I hope I got it right this time @teddyzeenny :)

@Mawaheb Mawaheb closed this Jun 17, 2015
@Mawaheb Mawaheb reopened this Jun 17, 2015
this._super();
$(window).off(this.get('eventNamespace'));
$(layerDiv).remove();
$(previewDiv).remove();
this.get('_lastNodes').clear();
View.removeMutationListener(this.viewTreeChanged);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed.

@Mawaheb Mawaheb force-pushed the refactorEmberDebug branch from d768377 to 3cb4460 Compare June 22, 2015 11:23
@Mawaheb
Copy link
Contributor Author

Mawaheb commented Jun 22, 2015

I made the changes you mentioned, Please let me know if there are any further changes that should be made.

teddyzeenny added a commit that referenced this pull request Jun 22, 2015
- Refactor ember-debug to use ES6.
@teddyzeenny teddyzeenny merged commit 664b3c2 into emberjs:master Jun 22, 2015
@teddyzeenny
Copy link
Contributor

Thanks @Mawaheb!

@Mawaheb
Copy link
Contributor Author

Mawaheb commented Jun 22, 2015

my pleasure :).

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