search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] [ARCH] Include jsTree as submodule #3574

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by TuckerWhitehouse Tuesday May 14, 2013 at 02:18 GMT Originally opened as https://github.com/adobe/brackets/pull/3817

Includes jsTree as a submodule (same as current version - v.pre1.0) and polyfills $.fn.addBack until jQuery 2.0.0 lands (at which time upgrading to jsTree 2.0 may also be possible).

TuckerWhitehouse included the following code: https://github.com/adobe/brackets/pull/3817/commits

core-ai-bot commented 3 years ago

Comment by peterflynn Tuesday May 14, 2013 at 02:37 GMT

I believe we made several bugfixes to our local copy of jsTree -- do you have a diff of the copy in Brackets vs. the bits in the submodule? (Or just looking at git log in that subtree may be useful).

core-ai-bot commented 3 years ago

Comment by TuckerWhitehouse Tuesday May 14, 2013 at 18:45 GMT

I've got a diff at https://github.com/TuckerWhitehouse/brackets/compare/jstree_diff. The only contribution I see from brackets is on line 1112 but everything else seems to be upstream changes to whitespace, the use of .addBack instead of .andSelf and a bug fix for IE 9 (that doesn't seem to break anything in brackets).

As for the change on line 1112, on OS X Mountain Lion, I don't see any issues with scrolling using the upstream (submodule) version, not sure about different versions of OS X or Windows.

core-ai-bot commented 3 years ago

Comment by WebsiteDeveloper Tuesday May 14, 2013 at 18:56 GMT

@TuckerWhitehouse the inline styles on line 1279 were also moved out, in fact this was my first contribution to brackets.

core-ai-bot commented 3 years ago

Comment by TuckerWhitehouse Tuesday May 14, 2013 at 20:09 GMT

@WebsiteDeveloper oops, I was just looking for deletions / brackets comments, so I didn't notice the additions there. Is there any harm to leaving these in until jQuery & jstree get updated?

core-ai-bot commented 3 years ago

Comment by WebsiteDeveloper Tuesday May 14, 2013 at 20:48 GMT

i don't think it does really matter, because those styles weren't changed in brackets as far as i know. It was just a code cleanup, because setting those styles there is a bit of a bad practice.

core-ai-bot commented 3 years ago

Comment by jasonsanjose Tuesday May 14, 2013 at 22:43 GMT

Here's the pull request from last year where _fix_scroll was commented out. https://github.com/adobe/brackets/pull/784.

I believe the behavior we're trying to avoid is present in this pull request. If you hover over a tree item that is clipped vertically (top or bottom), it scrolls the viewport. It doesn't seem that bad, but for some reason at the bottom of the viewport you can hit a sequence of scrolls and eventually hit the bottom of the viewport for no good reason.

core-ai-bot commented 3 years ago

Comment by jasonsanjose Tuesday May 14, 2013 at 23:00 GMT

@TuckerWhitehouse even after we resolve the jQuery version, there's still the patch for #784 that we would need to address. I could bring this up in our next architecture meeting. Off the top of my head, we could consider forking the jstree project and applying a patch.

core-ai-bot commented 3 years ago

Comment by TuckerWhitehouse Wednesday May 15, 2013 at 01:16 GMT

@jasonsanjose When jQuery is upgraded, the jsTree submodule could be updated as well, and I think the scrolling bug may be eliminated (have to double check on that), but forking may be a better option.

core-ai-bot commented 3 years ago

Comment by jasonsanjose Monday Jul 15, 2013 at 17:43 GMT

I'll take a new look at this now that jQuery 2.0 landed.

core-ai-bot commented 3 years ago

Comment by jasonsanjose Monday Aug 12, 2013 at 20:55 GMT

Closing. Thanks@TuckerWhitehouse for the submission though.

We talked about this during our sprint planning today. Since it doesn't appear that jstree is actively maintained anymore it would be more trouble for us to change to a submodule presuming we need to apply monkey patches in the future.