noprompt / ankha

A data inspection component for Om
Eclipse Public License 1.0
132 stars 12 forks source link

Pagination #16

Closed maxweber closed 10 years ago

maxweber commented 10 years ago

Hi,

first of all thank you for creating this great tool. We have been using it right from the start in our project and it worked perfectly. However as soon as we added a lot of items in the Om app state, ankha started to become more and more unresponsive until the point where it crashed the browser.

The pull request contains two changes. The first change cfe77bcc64637d70a41021b6a96503fd0b5bb59a takes care that the data of the parent element is not over and over repeated in the data-reactid attributes of its child elements. With this change the browser crash could be avoided with the size of our Om app state.

But the browser is still very unresponsive until the rendering is done. Therefore the second change c14b7b346c3a91ec4773fb9cc2070825ba70bc55 adds a pagination feature to ankha that enables it to stay responsive even if the Om app states contains hundreds or thousands of items.

I hope my formatting and the coding style is okay. If you like to have any changes I'm happy to include them. I would be glad, if this feature would make it into the ankha master (maybe as optional feature).

Best regards

Max

noprompt commented 10 years ago

Thanks for putting this together. I've noticed the same problem with some of our applications but hadn't come up with a solution. I'll check out the branch and play around with it and get back to you.

noprompt commented 10 years ago

I just had an opportunity to try this out. It's awesome! This solves a real problem several people have reported and, as mentioned, I've had it too. I made a few small tweaks to the code for sequential->dom

(defn sequential->dom
  [data owner {:keys [page-item-count] :or {page-item-count 10}}]
  (reify
    om/IInitState
    (init-state [_]
      {:page 0})

    om/IRenderState
    (render-state [_ {:keys [page]}]
      (let [button-style {:display "inline-block"
                          :verticalAlign "top"
                          :border "none"
                          :background "none"
                          :cursor "pointer"
                          :outline "none"
                          :fontWeight "bold"
                          :padding "0 1em"}
            total (count data)
            total-pages (max 1 (dec (js/Math.ceil (/ total page-item-count))))
            first-page? (zero? page)
            last-page? (= page total-pages)]

        (dom/div nil
         (dom/button
          #js {:onClick
               (fn [_]
                 (when-not first-page?
                   (om/update-state! owner :page dec)))
               :style (clj->js
                       (assoc button-style :opacity (if first-page? "0.3" "1.0")))}
          "Prev")

         (dom/span nil (str "Page " page " of " total-pages " (" total " items)"))

         (dom/button
           #js {:onClick
                (fn [_]
                  (when-not last-page?
                    (om/update-state! owner :page inc)))
                :style (clj->js
                        (assoc button-style :opacity (if last-page? "0.3" "1.0")))}
           "Next")

         (let [page-data (->> data
                              (drop (* page page-item-count))
                              (take page-item-count))]
           (into-array
            (for [[i x :as pair] (map-indexed vector page-data)]
              (dom/li #js {:className "entry"
                           :key (hash-key pair)}
                      (inspect x))))))))))

This cleans up a few things so we don't have count in the component and moves the logic outside of the om/update-state! functions. I've also made the numbers a bit easer to understand with a placeholder (e.g. Page 36 of 99 (1000 items)). If you want I can apply this after I merge your patch or you can apply this to your pull request; let me know either way.

Thanks!

maxweber commented 10 years ago

I'm happy that you like this feature :-) I guess it is the easiest way, if you merge your tweaks after applying my pull request. I only discovered one minor issue, that it is still possible to hit the "Next" button, if you have a collection with equal or less than 10 items (since then total-pages is 1, page is 0 and therefore last-page? is false). Below is a version of sequential->dom that fix this issue. Furthermore it starts counting pages with 1 instead of 0, which I would say is more intuitive in the context of pagination.

(defn sequential->dom
  [data owner {:keys [page-item-count] :or {page-item-count 10}}]
  (reify
    om/IInitState
    (init-state [_]
      {:page 1})

    om/IRenderState
    (render-state [_ {:keys [page]}]
      (let [button-style {:display "inline-block"
                          :verticalAlign "top"
                          :border "none"
                          :background "none"
                          :cursor "pointer"
                          :outline "none"
                          :fontWeight "bold"
                          :padding "0 1em"}
            total (count data)
            total-pages (js/Math.ceil (/ total page-item-count))
            first-page? (= page 1)
            last-page? (= page total-pages)]

        (dom/div nil
         (dom/button
          #js {:onClick
               (fn [_]
                 (when-not first-page?
                   (om/update-state! owner :page dec)))
               :style (clj->js
                       (assoc button-style :opacity (if first-page? "0.3" "1.0")))}
          "Prev")

         (dom/span nil (str "Page " page " of " total-pages " (" total " items)"))

         (dom/button
           #js {:onClick
                (fn [_]
                  (when-not last-page?
                    (om/update-state! owner :page inc)))
                :style (clj->js
                        (assoc button-style :opacity (if last-page? "0.3" "1.0")))}
           "Next")

         (let [page-data (->> data
                              (drop (* (dec page) page-item-count))
                              (take page-item-count))]
           (into-array
            (for [[i x :as pair] (map-indexed vector page-data)]
              (dom/li #js {:className "entry"
                           :key (hash-key pair)}
                      (inspect x))))))))))

I'm not exactly sure, if it is always consistent to calculate first-page? and last-page? outside of om/update-state!. I moved the check there because I had a bug that you can press "<<" very fast and enter a state where I have a negative page number. However I was not able to reproduce this issue with the new version of sequential->dom.

Best regards

Max

noprompt commented 10 years ago

I'm not exactly sure, if it is always consistent to calculate first-page? and last-page? outside of om/update-state!

We will always be in a consistent state when those get calculated so it shouldn't be a problem.

I'll merge this now and push out a new "snapshot" release. Thanks again! You're awesome! :smile:

noprompt commented 10 years ago

@maxweber You can depend on [ankha "0.1.5.1-8f1268"] to take advantage of these changes.

maxweber commented 10 years ago

:smile: Thank you for being so awesome to create this great tool. Ankha is really a missing tool in the ClojureScript toolbox, also because there is no official clojure.pprint port right now.

noprompt commented 10 years ago

@maxweber You might also check out Clairvoyant. It doesn't do pretty printing; it does tracing. It can be used alongside Ankha and even with Om. Try it out!