Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

[ARCH] Include jsTree as submodule #3817

Closed
wants to merge 4 commits into from
Closed

[ARCH] Include jsTree as submodule #3817

wants to merge 4 commits into from

Conversation

TuckerWhitehouse
Copy link
Contributor

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).

@@ -119,6 +119,9 @@
<!-- All other scripts are loaded through require. -->
<script src="thirdparty/requirejs/require.js" data-main="brackets"></script>

<!-- Polyfil $.fn.addBack until jQuery 2.0.0 lands -->
<script>$.fn.addBack = $.fn.andSelf;</script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better place to polyfill this for jsTree until jQuery is upgraded to 2.0.0?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a better place? Not that I can think of. I think we're better off waiting for the jQuery upgrade. Otherwise, the only other idea I've got is to somehow load multiple versions of jQuery simultaneously. It appears this is possible, but I don't think we want to get into that territory.

@peterflynn
Copy link
Member

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).

@ghost ghost assigned jasonsanjose May 14, 2013
@TuckerWhitehouse
Copy link
Contributor Author

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.

@WebsiteDeveloper
Copy link
Contributor

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

@TuckerWhitehouse
Copy link
Contributor Author

@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?

@WebsiteDeveloper
Copy link
Contributor

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.

@jasonsanjose
Copy link
Member

Here's the pull request from last year where _fix_scroll was commented out. #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.

@jasonsanjose
Copy link
Member

@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.

@TuckerWhitehouse
Copy link
Contributor Author

@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.

@jasonsanjose
Copy link
Member

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

@jasonsanjose
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants