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 jQuery from component lifecycle #200

Merged
merged 7 commits into from Nov 10, 2018

Conversation

Projects
None yet
3 participants
@jkarsrud
Contributor

jkarsrud commented Oct 8, 2018

This fixes #197, fixes #198 and fixes #199.

I'm not 100% sure if we should be using the date picker example anymore, but I kept it as a straight conversion right now in order for the diffs to be easier to see and review.

@kamry-bowman

I worked on this too, missed you had put this together! I'm happy to use yours, but here are my notes based on my work on it...

@@ -246,7 +247,7 @@ export default Component.extend({
didRender() {
this._super(...arguments);
this.$('.item-list').scrollTop(this.$('.selected-item').position().top);
this.element.querySelector('.item-list').scrollTop = this.element.querySelector('.selected-item').offsetTop);

This comment has been minimized.

@kamry-bowman

kamry-bowman Oct 8, 2018

I'm not sure this works, as offsetTop will use the document or nearest positioned element. Not sure you can assume that the .item-list would be positioned. Here is how I tackled this:

const scrollTarget = Math.abs(this.element.getBoundingClientRect().top - this.element.querySelector('.selected-item').getBoundingClientRect().top);
this.element.querySelector('.item-list').scrollTop = scrollTarget;   

Here is a code pen illustrating...
https://codepen.io/kamry-bowman/pen/MPJaRo?editors=1011

@@ -272,8 +273,8 @@ import Component from '@ember/component';
export default Component.extend({
willDestroyElement() {
this.$().off('animationend');
this.$('input.date').myDatepickerLib().destroy();
this.element.removeEventListener('animationend');

This comment has been minimized.

@kamry-bowman

kamry-bowman Oct 8, 2018

I think removeEventListener takes a second argument, which needs to be a function reference. This poses a problem, as we would need to have a reference to the original function we put on the element with addEventListener. Don't know how we want to handle that?

This comment has been minimized.

@jkarsrud

jkarsrud Oct 9, 2018

Contributor

You're absolutely right. I think we could get away with just creating the handler in the example that adds the event listener, and just using that in this sample. The samples in this file aren't really connected as a whole, so we can probably get away with some "cheating".

@jenweber

It would be helpful to have a twiddle that shows that these selectors work as shown in the guides.

I also added on small edit for a url. Thank you!

@@ -200,7 +201,7 @@ There are a few things to note about the `didInsertElement()` hook:
particularly when order of execution is important.
[did-insert-element]: https://www.emberjs.com/api/ember/release/classes/Component/events/didInsertElement?anchor=didInsertElement
[dollar]: https://www.emberjs.com/api/ember/release/classes/Component/methods/$?anchor=%24
[element]: https://emberjs.com/api/ember/3.4/classes/Component/properties/element?anchor=element

This comment has been minimized.

@jenweber

jenweber Oct 9, 2018

Contributor

Can you change this link to use /release/ instead of /3.4/ in the url? Thanks!

@jenweber

This comment has been minimized.

Contributor

jenweber commented Oct 9, 2018

Hi! This PR was still open as of creating the 3.5 release. This means before this PR to be merged, any changes requested should be made, and then the same additions should be applied to the upcoming 3.5 version files. They will be available later this week. Sorry about the extra step!

@jkarsrud

This comment has been minimized.

Contributor

jkarsrud commented Oct 9, 2018

Don't worry about the extra step, I cause some extra steps for you when not commenting on the issues too! I'll make a Twiddle to show that it all works, and then wait for the 3.5 files to land.

@jenweber

This comment has been minimized.

Contributor

jenweber commented Oct 13, 2018

@jkarsrud The 3.5 files are up. Thanks!

@jenweber

This comment has been minimized.

Contributor

jenweber commented Oct 27, 2018

@jkarsrud hey, do you still want to make these last changes and add them to 3.5 too?

jkarsrud added some commits Oct 8, 2018

@jkarsrud

This comment has been minimized.

Contributor

jkarsrud commented Oct 30, 2018

@jenweber Sorry about the delay. I've updated the exaples and copied the changes over to the 3.5.0 files. Maybe you want me to revert the 3.4.0 files, so that we only update the released guides?

I have also made an Ember Twiddle that shows that some of these things work as expected: https://ember-twiddle.com/c8c9f62aa2bc9f7807edf89f8b246bfe?numColumns=2&openFiles=components.my-component.js%2Ctemplates.components.my-component.hbs

I will update the twiddle with an example component for the more complex examples we have in these files.

Let me know if there's anything else you want me to do in this PR!

@jkarsrud

This comment has been minimized.

Contributor

jkarsrud commented Oct 30, 2018

The Twiddle is updated with working examples for "everything" now. While implementing those examples, I noticed that the code actually didn't work as expected, but I'm not sure the fix is what we should be showing in these examples...

@jenweber

This comment has been minimized.

Contributor

jenweber commented Nov 10, 2018

This is awesome, thank you! I appreciate the Twiddles. Merged.

@jenweber jenweber merged commit 4e43623 into ember-learn:master Nov 10, 2018

1 check passed

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