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

Remove the use of Jquery in Vue objects #422

Merged
merged 2 commits into from
May 9, 2017

Conversation

martinRenou
Copy link
Member

No description provided.

@codecov-io
Copy link

codecov-io commented May 4, 2017

Codecov Report

Merging #422 into switch_to_vue will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           switch_to_vue     #422   +/-   ##
==============================================
  Coverage          95.24%   95.24%           
==============================================
  Files                 96       96           
  Lines               4102     4102           
  Branches             255      255           
==============================================
  Hits                3907     3907           
  Misses               143      143           
  Partials              52       52

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3638482...925870a. Read the comment docs.

@@ -51,5 +51,7 @@ require([
ga_observer.trigger_application_starting(application.app_data.image.name);
});

app_list_view.$on('focus_iframe', function() { app_view.focus_iframe(); });
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just check on click event instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried, it didn't work. So I just renamed the event to entry_clicked

@@ -51,5 +51,7 @@ require([
ga_observer.trigger_application_starting(application.app_data.image.name);
});

app_list_view.$on('entry_clicked', function() { app_view.focus_iframe(); });
Copy link
Contributor

Choose a reason for hiding this comment

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

only clicked. You don't need another event. clicked is already an event.

Copy link
Member Author

@martinRenou martinRenou May 8, 2017

Choose a reason for hiding this comment

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

I can rename it clicked, but I will still need to emit the event with the $emit method. You want me to rename it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said in the other comment, it sounds like "clicked" or "click" are not built-in events for the $on method of Vue. I think it's not really related to the javascript method addEventListener("click", myFunction). We can only use the $onmethod with custom events and trigger them with the $emit method.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a design, it makes no sense, but we can't do otherwise. I'd say add a comment as a reminder if someone is confused for the same reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also tried app_list_view.$on('click.native', myFunction) according to the documentation but it sounds like the .native modifier doesn't work for $on, but only for v-on in templates.

@martinRenou
Copy link
Member Author

@stefanoborini Ok to merge or you would prefer to rename the event from entry_clicked to clicked?

@@ -51,5 +51,7 @@ require([
ga_observer.trigger_application_starting(application.app_data.image.name);
});

app_list_view.$on('entry_clicked', function() { app_view.focus_iframe(); });
Copy link
Contributor

Choose a reason for hiding this comment

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

As a design, it makes no sense, but we can't do otherwise. I'd say add a comment as a reminder if someone is confused for the same reason.

@stefanoborini stefanoborini merged commit fed2eca into switch_to_vue May 9, 2017
@stefanoborini stefanoborini deleted the remove_jquery_from_vue branch May 9, 2017 15:43
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