Remove usages of jQuery #98

donaldwasserman · 2018-10-30T16:35:45Z

note: I am waiting to update the tests because of

addon/components/ember-scrollable.js

addon/services/scrollbar-thickness.js

alexander-alvarez

When you do the change if you could verify locally that would be great.
Apparently we don't have a test that verifies that the scrollbar actually renders 😢

addon/classes/scrollable.js

donaldwasserman · 2018-11-14T16:06:21Z

@alexander-alvarez So I went through and tried best I could to compare a lot of the measurements between the jquery/non-jqery version.

This appears to work for me locally, although I'm really not sure why the tests were passing, because it seems like they shouldn't.

alexander-alvarez · 2018-11-14T18:34:16Z

addon/components/ember-scrollable.js

@@ -228,8 +227,8 @@ export default Component.extend(InboundActionsMixin, DomMixin, {
  },

  resizeScrollContent() {
-    const width = this.$().width();
-    const height = this.$().height();
+    const width = this.element.offsetWidth;


I'm a little confused now, why did this one switch to offset and why did https://github.com/alphasights/ember-scrollable/pull/98/files#diff-3fa203c13b3ee2249dcb66b55d4c297fR22 switch to getHeight?

Is that simply what you needed to do to get it working?

I wish I could say I sat down and did the math to figure out all the details of why and how each piece interacted, but I basically just put a few console logs across the critical paths and both this branch and master, and this is the measurement that worked.

If you could try it out too, that would be great, but it does seem like it's working.

I'd like to add another test to make sure the onScrolledToBottom action gets called, because that should have been failing, but wasn't. However, I'm not 100% sure how to get the elements to scroll.

I haven't forgotten about this, just haven't gotten around to it. Hopefully before or during this coming Thanksgiving holiday.

so yeah... I forgot ....

donaldwasserman · 2019-01-22T15:46:22Z

Hey @alexander-alvarez I sort of lost track of this over the holidays and am now resuming my quest to remove jquery from Ember-light-table.

What do you think we need to check to get those over the finish line?

alexander-alvarez · 2019-04-19T19:11:48Z

I think I'd want some reasoning or extensive testing on many browsers to make sure this is a.o.k given the discrepancy mentioned above. This space is filled with subtitles that I am pretty unfamiliar with that I'm hesitant to make changes to it without knowing it will be g2g for our downstream consumers

donaldwasserman · 2019-04-19T23:54:22Z

You know, I don’t remember a single thing about this PR. Looking back through it, I’ll just redo everything with the getHeight replacement.

I have an app that depends pretty heavily on ELT, so getting this done correctly is a high priority.

alexander-alvarez · 2019-04-22T15:16:19Z

Apologies for delaying you @donaldwasserman

donaldwasserman · 2019-04-22T18:32:50Z

@alexander-alvarez no prob! I was really motivated to get rid of jQuery from my app in december, but then lost interest when i realized how much i depended on liquid fire, lol. I'll hopefully have some time this week to get this wrapped up.

richard-viney · 2019-08-17T12:03:11Z

@donaldwasserman Is the only thing remaining here the more consistent use of the getHeight replacement function?

donaldwasserman · 2019-08-17T13:39:14Z

Hey @richard-viney - Thanks for the bump on this. I actually have this somewhat working locally. But the issue I'm having some outdated test dependencies. So I'm just trying to get that fixed up this morning and pushed up.

donaldwasserman · 2019-08-17T15:12:03Z

@richard-viney, @alexander-alvarez another quick update, I switched up some various things:

I added back jquery & non-jquery tests so we can have some certainty about the test coverage
Now there's some failing tests that I can't quite figure out why, and have run out of time this AM to work on it. I'll pick it back up hopefully next week.

If anyone has any suggestions or notices a quick fix, let me know.

alvinvogelzang · 2019-12-14T16:47:32Z

@donaldwasserman you accidentally changed firstMovement to secondMovement in scroll-content-element-test.js. I've fixed it in the following PR: donaldwasserman#1

All the ember tests succeed locally but somehow the tests in Travis are failing but I'm not sure what to do about that.

donaldwasserman · 2019-12-16T17:12:29Z

@alexander-alvarez - Looks like this should be good to go now, but i don't see travis reporting anything on here anymore. I know this is sort of off the radar for you, but any ideas? It would be nice to get this wrapped up, and desipite it taking me a year plus on this project, i'd be happy to take this over for you and get the outstanding PR's fixed up.

Or @alexander-alvarez, we could move this whole repo over to the adopted-ember-addons thing.

alexander-alvarez · 2019-12-16T18:16:36Z

@donaldwasserman not sure what is going on with travis will have to take a closer look. I agree it would be nice to get this wrapped up & thank you for your continued contributions.

Also I'm in favor of moving this to an area where it gets more love. Unfortunately I'm not the project maintainer, and I don't know who is. It seems the listed maintainer left Alphasights a while ago @eugeniodepalo any ideas as to who succeeded you?

donaldwasserman · 2019-12-16T19:47:50Z

@alexander-alvarez Not sure what the deal with Jenkins either, but it looks like there may be some random errors left: https://travis-ci.com/donaldwasserman/ember-scrollable

RobbieTheWagner · 2020-01-11T15:18:16Z

@donaldwasserman @alexander-alvarez anything I can do to help with these efforts? Ember-light-table, which uses ember-scrollable, is pretty much the only thing left in our app using jQuery.

donaldwasserman · 2020-01-11T18:52:19Z

So the only error left is on the latest ember versions, and I think it’s an issue in the dummy app itself, but that’s just a wild guess.

I’ll set aside some time on Monday to investigate

RobbieTheWagner · 2020-01-11T20:04:59Z

@donaldwasserman where can I see the errors? Happy to take a look.

donaldwasserman · 2020-01-11T21:23:36Z

If you want to check it out it’s right here: https://travis-ci.com/donaldwasserman/ember-scrollable

I don’t have access to this repo, so I don’t know why Travis has disappeared here

I don’t have my computer in front of me, but basically the error is something like “cannot getOwner of undefined”

RobbieTheWagner · 2020-01-11T23:45:39Z

@donaldwasserman ah, looks like it is saying the owner is undefined when it tries to use .lookup on it. Are the tests using the new testing syntax?

donaldwasserman · 2020-01-12T03:11:33Z

Thanks @rwwagner90. I was trying to not touch any of the test code (since i'm just dropping in on this...uh...2 years ago)

I'll run the codemod on that see what happens

donaldwasserman · 2020-01-12T04:21:14Z

Thanks for the pointer @rwwagner90 - things are working on my machine ™️ .

Here's travis: https://travis-ci.com/donaldwasserman/ember-scrollable/builds/144037283

(Note, I didn't actually wait around for this to build, but am going to bed. So it may or may not actually be passing by the time anyone looks at this)

Narrator: It Didn't Work

But this one may: https://travis-ci.com/donaldwasserman/ember-scrollable/builds/144037363

RobbieTheWagner · 2020-01-12T12:14:30Z

@donaldwasserman looks like everything is green! 🎉

donaldwasserman · 2020-01-12T13:12:20Z

Thanks for the bump @rwwagner90.

@alexander-alvarez See #121 where i added github actions for tests

I'll squash all the commits in here after that gets merged so we get a clean test run, then I think we can talk about pushing this out & cutting a release for downstream

RobbieTheWagner · 2020-01-12T16:44:34Z

Thanks for all your work on this @donaldwasserman!

donaldwasserman · 2020-01-14T14:33:04Z

Hey @alexander-alvarez can you take a second to review + merge + maybe cut a new release?

eelke · 2020-01-14T16:36:28Z

@donaldwasserman I tried out the latest commit from your feature branch in a project that I'm working on, but it doesn't work correctly: the drag handle cannot be dragged, and clicking on the scrollbar works neither.

I just ran the dummy app from that same commit and confirmed that this problem exists there as well.

donaldwasserman · 2020-01-14T18:29:05Z

thanks @eelke - thanks for pointing it out should be fixed now. Must have missed it some of the rebasing i did.

addon/components/ember-scrollable.js

alexander-alvarez · 2020-01-15T20:31:59Z

@donaldwasserman thanks for all the work on this, once the typo update is in and side scrolling should work, hence this PR should finally be G2G

donaldwasserman · 2020-01-16T02:01:57Z

@alexander-alvarez sorry about that. TV + bug fixing don't mix. We shoudl figure out a way to add a test for that whole flow anyway...

RobbieTheWagner · 2020-01-16T19:18:02Z

Now that this is merged, can we release a new version and update ember-light-table?

alexander-alvarez · 2020-01-16T19:20:56Z

We've released https://github.com/alphasights/ember-scrollable/releases/tag/v1.0.0
Thanks again @donaldwasserman and everyone who's helped get this done

alexander-alvarez · 2020-01-16T19:36:13Z

@donaldwasserman @rwwagner90 see https://github.com/offirgolan/ember-light-table/releases/tag/2.0.0-beta.5

donaldwasserman · 2020-01-16T19:42:07Z

@alexander-alvarez Thanks a bunch! Glad we've finally got this across the finish line.

donaldwasserman mentioned this pull request Oct 30, 2018

[Tracking] Remove jQuery adopted-ember-addons/ember-light-table#594

Closed

3 tasks

alexander-alvarez reviewed Nov 2, 2018

View reviewed changes

addon/components/ember-scrollable.js Outdated Show resolved Hide resolved

alexander-alvarez reviewed Nov 2, 2018

View reviewed changes

addon/components/ember-scrollable.js Outdated Show resolved Hide resolved

alexander-alvarez reviewed Nov 2, 2018

View reviewed changes

addon/services/scrollbar-thickness.js Outdated Show resolved Hide resolved

alexander-alvarez mentioned this pull request Nov 2, 2018

Bump ember-element-resize-detector to fix sendAction deprecation warning #99

Closed

alexander-alvarez suggested changes Nov 13, 2018

View reviewed changes

addon/classes/scrollable.js Outdated Show resolved Hide resolved

alexander-alvarez reviewed Nov 14, 2018

View reviewed changes

alexander-alvarez mentioned this pull request Apr 15, 2019

jQuery dependency #105

Open

donaldwasserman force-pushed the feat/remove-jquery branch from a0f0416 to 81b97d7 Compare August 17, 2019 14:33

alvinvogelzang mentioned this pull request Dec 14, 2019

Fix integration test donaldwasserman/ember-scrollable#1

Closed

alexander-alvarez closed this Dec 16, 2019

alexander-alvarez reopened this Dec 16, 2019

donaldwasserman force-pushed the feat/remove-jquery branch from 7324124 to 83d04c2 Compare December 16, 2019 19:45

donaldwasserman force-pushed the feat/remove-jquery branch from b3962bc to 9192acf Compare January 12, 2020 04:20

donaldwasserman mentioned this pull request Jan 12, 2020

Consider Putting Up For Adoption #122

Open

donaldwasserman requested a review from alexander-alvarez January 13, 2020 20:53

donaldwasserman force-pushed the feat/remove-jquery branch from fff38fe to 5972cb2 Compare January 14, 2020 00:46

alexander-alvarez reviewed Jan 15, 2020

View reviewed changes

addon/components/ember-scrollable.js Outdated Show resolved Hide resolved

Remove jQuery, refactor tests to new style, various upgrades

54a4f95

donaldwasserman force-pushed the feat/remove-jquery branch from 396d4fa to 54a4f95 Compare January 16, 2020 02:01

alexander-alvarez merged commit 084c9d6 into alphasights:master Jan 16, 2020

Remove usages of jQuery #98

Remove usages of jQuery #98

Conversation

donaldwasserman commented Oct 30, 2018

alexander-alvarez left a comment

Choose a reason for hiding this comment

donaldwasserman commented Nov 14, 2018

alexander-alvarez Nov 14, 2018

Choose a reason for hiding this comment

donaldwasserman Nov 15, 2018

Choose a reason for hiding this comment

alexander-alvarez Nov 20, 2018

Choose a reason for hiding this comment

alexander-alvarez Dec 11, 2018

Choose a reason for hiding this comment

donaldwasserman commented Jan 22, 2019

alexander-alvarez commented Apr 19, 2019

donaldwasserman commented Apr 19, 2019

alexander-alvarez commented Apr 22, 2019

donaldwasserman commented Apr 22, 2019

richard-viney commented Aug 17, 2019 • edited Loading

donaldwasserman commented Aug 17, 2019

donaldwasserman commented Aug 17, 2019

alvinvogelzang commented Dec 14, 2019

donaldwasserman commented Dec 16, 2019 • edited Loading

alexander-alvarez commented Dec 16, 2019

donaldwasserman commented Dec 16, 2019

RobbieTheWagner commented Jan 11, 2020

donaldwasserman commented Jan 11, 2020

RobbieTheWagner commented Jan 11, 2020

donaldwasserman commented Jan 11, 2020 • edited Loading

RobbieTheWagner commented Jan 11, 2020

donaldwasserman commented Jan 12, 2020

donaldwasserman commented Jan 12, 2020 • edited Loading

RobbieTheWagner commented Jan 12, 2020

donaldwasserman commented Jan 12, 2020

RobbieTheWagner commented Jan 12, 2020

donaldwasserman commented Jan 14, 2020

eelke commented Jan 14, 2020

donaldwasserman commented Jan 14, 2020

alexander-alvarez commented Jan 15, 2020 • edited Loading

donaldwasserman commented Jan 16, 2020

RobbieTheWagner commented Jan 16, 2020

alexander-alvarez commented Jan 16, 2020

alexander-alvarez commented Jan 16, 2020

donaldwasserman commented Jan 16, 2020

richard-viney commented Aug 17, 2019 •

edited

Loading

donaldwasserman commented Dec 16, 2019 •

edited

Loading

donaldwasserman commented Jan 11, 2020 •

edited

Loading

donaldwasserman commented Jan 12, 2020 •

edited

Loading

alexander-alvarez commented Jan 15, 2020 •

edited

Loading