Skip to content

Conversation

@frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented May 18, 2021

Thanks @jacekschae for this repo. I'm new to re-frame and it gave me a good idea of what it provides.

I found some possible inconsistencies which I tried to address in this PR.

  1. conduit.events/add-epoch: I didn't understand how the date parameter was being used so I removed it, and the docstring seemed out of date.
  2. conduit.events/index-by: Why did it pass :createdAt to add-epoch?
  3. :loading: fixed :loading entries in events (they didn't always match up)
  4. :api-request-error: more fixes for :loading updates, now takes an optional arg for the :loading key
  5. Small style changes (avoid shadowing some clojure.core vars, use transducers, use update over update-in)
  6. Fix :disabled usage in views

Please let me know which (if any) of these changes you're interested in, or if I've misunderstood something.

@frenchy64
Copy link
Contributor Author

After sleeping on it, I feel I'm missing what add-epoch is supposed to do, and how index-by is supposed to use it. I'd welcome clarification.

@jacekschae
Copy link
Owner

Thanks for this PR! Didn't have a chance to look at the changes.

To clarify add-epoch it is converting dates to unix timestamps so that when we display the posts (the initially downloaded ones 10 or 20) they are displayed in order. index-by adds this prop when it converts [] to {} so later it's trivial to navigate.

When we get the blog posts from the API one of the properties is - :createdAt "2021-05-20T11:31:13.731Z" and we use this information to add additional property :epoch 1621510273731. Then in the subscription we sort them by :epoch so they appear in order --

(reg-sub
:articles ;; usage: (subscribe [:articles])
(fn [db _] ;; db is the (map) value stored in the app-db atom
(->> (:articles db) ;; ->> is a thread last macro - pass atricles as last arg of:
(vals) ;; vals, just as we would write (vals articles), then pass the result to:
(sort-by :epoch reverse-cmp)))) ;; sort-by epoch in reverse order

Take a look at the re-frame-10x-demo. Press CTRL + h, if the re-frame 10x panel is not visible. There when you expand articles you will see :epoch along :createdAt.

Screen Shot 2021-05-20 at 14 15 22

Hope that helps

@frenchy64
Copy link
Contributor Author

frenchy64 commented May 20, 2021

Ah! After a few iterations, I think this works:

(defn add-epoch
  "Add :epoch timestamp based on :createdAt field."
  [item]
  (assoc item :epoch (-> item :createdAt rdr/parse-timestamp .getTime)))

(defn index-by
  "Index collection by function f (usually a keyword) as a map"
  [f coll]
  (into {}
        (map (fn [item]
               (let [item (add-epoch item)]
                 [(f item) item])))
        coll))

I added it to the PR when you can take a look. Thanks!

@jacekschae jacekschae merged commit 9a481fb into jacekschae:master Nov 13, 2021
@jacekschae
Copy link
Owner

Thanks so much for this! Great updates across the board!

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