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

Fix failing tests without jQuery #4

Merged
merged 7 commits into from
Oct 13, 2015
Merged

Fix failing tests without jQuery #4

merged 7 commits into from
Oct 13, 2015

Conversation

Florian-R
Copy link

Finally had some time to work on #3. This PR is a WIP, tests still fails with the exo setup.

There's a few bug in the current version of test/initialize.js when tests are launched with the type=exos&useDeps=false query string.

I've fixed locally some of them, but i currently encouter a problem with the NativeView shim.

When NativeView replace BackboneView in the test setup, Chaplin.View has aleardy been required, so it extends Backbone.View instead of NativeView.

I'm a total noob with requireJS config, so any input is welcome

@@ -97,7 +97,7 @@ module.exports = class Layout extends View
openLink: (event) =>
return if utils.modifierKeyPressed(event)

el = if Backbone.$ then event.currentTarget else event.delegateTarget
el = event.currentTarget
Copy link
Author

Choose a reason for hiding this comment

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

Had to use currentTarget because delegateTarget didn't seems to have the same semantics in NativeView than its jQuery counterpart. Mind an issue in the NativeView repo?

Copy link
Owner

Choose a reason for hiding this comment

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

Not at all, thanks. I've been wondering about making the jump to use delegateTarget.

Copy link
Author

Choose a reason for hiding this comment

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

Might need to revert this commit. It's the cause of 8 failling tests with NativeView.

@akre54
Copy link
Owner

akre54 commented Feb 9, 2015

Merging as-is, will figure out the testing details later.

The Chaplin tests are pretty convoluted at the moment and not entirely easy to reason about. I'm pretty familiar with RequireJS, though, and happy to help. I have another branch (207487d) that has some work on this too.

@Florian-R
Copy link
Author

Nevermind the RequireJS bits, i've figured out a workaround. Some cleanup to do and i'll push for review.

@akre54
Copy link
Owner

akre54 commented Feb 9, 2015

Cool

@Florian-R
Copy link
Author

Done. Had to always use Underscore, all blew up without due to several calls to _.clone.

@Florian-R
Copy link
Author

Done. Needed to dome some weird stuff to define/undefine and then define again Underscore, else a circular dependancie caused Backbone to be empty.

@Florian-R
Copy link
Author

@akre54 I've finally had some time to fix all the failing tests. There's still a lot of jQuery branching through the specs and the codebase, but i'm not sure it's safe to remove $loadind, $list and all.

Also, the commits are messy, but i assume this branch will be rebased.

@Florian-R Florian-R changed the title Remove jQuery dependancie in Layout Fix failing tests without jQuery Sep 2, 2015
@Florian-R
Copy link
Author

@akre54 Friendly ping. Do you prefer that I open a new rebased PR against main Chapin repo?

akre54 added a commit that referenced this pull request Oct 13, 2015
Fix failing tests without jQuery
@akre54 akre54 merged commit 567c10c into akre54:bb-view-hooks Oct 13, 2015
@akre54
Copy link
Owner

akre54 commented Oct 13, 2015

Whatever works for you. I've also added you as a collaborator on my fork, so feel free to push any changes you want.

@Florian-R
Copy link
Author

Cool. Hopefully I'll could work on this next week, I'll see what fit the best at this time. Thanks a lot for your help on this.

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.

2 participants