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

More view hook changes #3

Open
akre54 opened this issue Jan 13, 2015 · 2 comments
Open

More view hook changes #3

akre54 opened this issue Jan 13, 2015 · 2 comments

Comments

@akre54
Copy link
Owner

akre54 commented Jan 13, 2015

I've had this sitting in my queue for a while, haven't had time to fully work out all the bugs. Here's a work-in-progress edit to the bb-view-hooks branch.

@Florian-R

diff --git a/src/chaplin/views/collection_view.coffee b/src/chaplin/views/collection_view.coffee
index 96ffc29..b7fa10c 100644
--- a/src/chaplin/views/collection_view.coffee
+++ b/src/chaplin/views/collection_view.coffee
@@ -5,6 +5,8 @@ Backbone = require 'backbone'
 View = require 'chaplin/views/view'
 utils = require 'chaplin/lib/utils'

+# jQuery's $.fn.toggle is much more advanced than simply hiding. We're including
+# it here because it'd be a huge block of code to include
 toggleElement = (elem, visible) ->
   if Backbone.$
     Backbone.$(elem).toggle visible
@@ -30,7 +32,7 @@ insertView = (list, viewEl, position, length, itemSelector) ->

   if insertInMiddle or itemSelector
     # Get the children which originate from item views.
-    children = list.querySelectorAll(itemSelector)
+    children = list.querySelectorAll itemSelector
     childrenLength = children.length

     # Check if it needs to be inserted.
@@ -203,7 +205,7 @@ module.exports = class CollectionView extends View

   # When an item is added, create a new view and insert it.
   itemAdded: (item, collection, options) =>
-    @insertView item, @renderItem(item), options.at
+    @insertView @listEl, item.el, @renderItem(item), options.at

   # When an item is removed, remove the corresponding view from DOM and caches.
   itemRemoved: (item) =>
@@ -354,10 +356,10 @@ module.exports = class CollectionView extends View
       view = @subview "itemView:#{item.cid}"
       if view
         # Re-insert the view.
-        @insertView item, view, index, false
+        @insertView @listEl, item.el, view, index, false
       else
         # Create a new view, render and insert it.
-        @insertView item, @renderItem(item), index
+        @insertView @listEl, item.el, @renderItem(item), index

     # If no view was created, trigger `visibilityChange` event manually.
     @trigger 'visibilityChange', @visibleItems if items.length is 0
@@ -404,7 +406,10 @@ module.exports = class CollectionView extends View

     # Start animation.
     if included and enableAnimation
-      startAnimation view.el, @useCssAnimation, @animationStartClass
+      if @useCssAnimation
+        view.el.classList.add @animationStartClass
+      else
+        view.el.style.opacity = 0

     # Hide or mark the view if it’s filtered.
     @filterCallback view, included if @filterer
@@ -427,7 +432,8 @@ module.exports = class CollectionView extends View
         setTimeout (=> view.el.classList.add @animationEndClass), 0
       else
         # Fade the view in if it was made transparent before.
-        endAnimation view.el, @animationDuration
+        view.el.style.transition = "opacity #{(@animationDuration / 1000)}s"
+        view.el.opacity = 1

     view

diff --git a/src/chaplin/views/layout.coffee b/src/chaplin/views/layout.coffee
index e21c81e..453bf94 100644
--- a/src/chaplin/views/layout.coffee
+++ b/src/chaplin/views/layout.coffee
@@ -203,18 +203,13 @@ module.exports = class Layout extends View

     # Apply the region selector.
     instance.container = if region.selector is ''
-      if Backbone.$
-        region.instance.$el
-      else
-        region.instance.el
+      region.instance.el
     else
-      if region.instance.noWrap
-        if Backbone.$
-          Backbone.$(region.instance.container).find region.selector
+      root = if region.instance.noWrap
+          region.instance.container
         else
-          region.instance.container.querySelector region.selector
-      else
-        region.instance.$ region.selector
+          region.instance.el
+      root.querySelector region.selector

   # Disposal
   # --------
diff --git a/test/spec/collection_view_spec.coffee b/test/spec/collection_view_spec.coffee
index f534f73..2743321 100644
--- a/test/spec/collection_view_spec.coffee
+++ b/test/spec/collection_view_spec.coffee
@@ -86,7 +86,7 @@ define [
         collectionView.$list.children collectionView.itemSelector
       else
         if collectionView.itemSelector
-          (item for item in collectionView.list.children when Backbone.utils.matchesSelector item, collectionView.itemSelector)
+          (item for item in collectionView.list.children when item.tagName is collectionView.itemSelector)
         else
           collectionView.list.children

@@ -609,11 +609,7 @@ define [
           model.get('title') is 'new'

         filterCallback = (view, included) ->
-          cls = if included then 'included' else 'not-included'
-          if jQuery
-            view.$el.addClass cls
-          else
-            view.el.classList.add cls
+          view.el.classList.add if included then 'included' else 'not-included'

         filterCallbackSpy = sinon.spy filterCallback
         collectionView.filter filterer, filterCallbackSpy
diff --git a/test/spec/layout_spec.coffee b/test/spec/layout_spec.coffee
index 70c46ac..daaf403 100644
--- a/test/spec/layout_spec.coffee
+++ b/test/spec/layout_spec.coffee
@@ -343,12 +343,8 @@ define [
       instance2 = new Test2View {region: 'test2'}
       instance3 = new Test2View {region: 'test0'}

-      if $
-        expect(instance2.container.attr('id')).to.be 'test2'
-        expect(instance3.container).to.be instance1.$el
-      else
-        expect(instance2.container.id).to.be 'test2'
-        expect(instance3.container).to.be instance1.el
+      expect(instance2.container.id).to.be 'test2'
+      expect(instance3.container).to.be instance1.el

       instance1.dispose()
       instance2.dispose()
@@ -375,10 +371,8 @@ define [
       instance1 = new Test1View()
       instance2 = new Test2View()
       instance3 = new Test3View {region: 'test2'}
-      if $
-        expect(instance3.container.attr('id')).to.be 'test5'
-      else
-        expect(instance3.container.id).to.be 'test5'
+
+      expect(instance3.container.id).to.be 'test5'

       instance1.dispose()
       instance2.dispose()
@@ -407,10 +401,8 @@ define [
       instance2 = new Test2View()
       instance2.stale = true
       instance3 = new Test3View {region: 'test2'}
-      if $
-        expect(instance3.container.attr('id')).to.be 'test2'
-      else
-        expect(instance3.container.id).to.be 'test2'
+
+      expect(instance3.container.id).to.be 'test2'

       instance1.dispose()
       instance2.dispose()
@Florian-R
Copy link

I like where it's going and i'd like to address some of these items. Thanks for sharing. Just a few thoughts/question.

  • I'm wondering what's the best approach for the animation in CollectionView. It's pretty easy for a user to subclass it to add className which fit the best for him, so maybe it's worth to remove all this logic (huge breaking change in that case, but it remove a lot of jQuery logic).
  • ClassList: Very cool to use, but it leaves IE < 10 out of the game. Is it acceptable?
  • It seems the signature of CollectionView#insertView has changed, but the corresponding changes are missing in this diff
  • Changes to Layout are pretty, but the tests seems to check on implementation details.

If i have the time, i 'll give it a shot next week, starting probably by Layout. In that case, do you prefer PRs on the same branch, or do you like to keep them separated of your initial PR?

@akre54
Copy link
Owner Author

akre54 commented Jan 23, 2015

ClassList: Very cool to use, but it leaves IE < 10 out of the game. Is it acceptable?

I'm of the opinion let's use natives where pollyfills are easily and readily available, and not worry too much about oldIE. We could also use a RegExp on className.

There will be b/c breakage here. It'd be nice to limit is as much as possible, but method signatures are a fine change imo.

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

No branches or pull requests

2 participants