search

viebel / klipse

Klipse is a JavaScript plugin for embedding interactive code snippets in tech blogs.
http://blog.klipse.tech/
GNU General Public License v3.0
3.11k stars 146 forks source link

Fixes #361 #372

Closed ghost closed 4 years ago

ghost commented 4 years ago

Fixes #361

ghost commented 4 years ago

Hi @viebel, what is the status of this PR?

ghost commented 4 years ago

2->1->3

On Fri, Jul 24, 2020, 8:29 PM Yehonathan Sharvit notifications@github.com wrote:

@viebel commented on this pull request.

In src/klipse/plugin.cljs https://github.com/viebel/klipse/pull/372#discussion_r460107617:

  • (if (seq elements)
  • (let [element (first elements)]
  • (let [eval-fn (<! (klipsify-no-eval element general-settings (modes element) #(go
  • (>! event-chan curr-idx))))]
  • (recur (rest elements) (conj eval-fns eval-fn) (inc curr-idx))))
  • eval-fns))]
  • (println eval-fns)
  • ; sub process that reloads other editors
  • (go
  • ; We reevaluate all the other snippets in the order they appear on page
  • ; code once a snippet is edited
  • (loop [idx (<! event-chan)]
  • ; evaluate edited snippet
  • ((eval-fns idx))
  • (if (general-settings :re_evaluate_all_snippets_on_change)

What is the order of evaluation now? Let's say we have 3 snippets on a page and snippet #2 https://github.com/viebel/klipse/pull/2 is edited?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/viebel/klipse/pull/372#pullrequestreview-454958761, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN5NKLZAMVLBD4GCIHUHBTLR5GOWVANCNFSM4OXPSJMQ .

viebel commented 4 years ago

@fctorial The ball is in your camp. Waiting for you to address the two remaining comments:

  1. https://github.com/viebel/klipse/pull/372#discussion_r460116526
  2. https://github.com/viebel/klipse/pull/372#discussion_r460116526
ghost commented 4 years ago

The reload order is now 2->1->2->3. An example mixing clojure and javascript has been added.

viebel commented 4 years ago

@fctorial Thank you for addressing the test page comment. One last comment is remaining: https://github.com/viebel/klipse/pull/372#discussion_r460116526

ghost commented 4 years ago

@viebel I've addressed that comment here: https://github.com/viebel/klipse/pull/372#issuecomment-667416466

I've tested that editors work as expected when the reload property is either true or false.

viebel commented 4 years ago

@fctorial Can you modify the code in src/klipse/plugin.cljs so that the code that handles the default case (:re_evaluate_all_snippets_on_change false) is not modified. I don't want to take any risk that it might cause regression. I don't mind if some code is duplicated.

viebel commented 4 years ago

Thank you for your contribution @fctorial

  1. Klipse version 7.10.0 includes your changes (deployed to prod)
  2. I slightly improved your code (see https://github.com/viebel/klipse/commit/31c4d9551f3ccda58a5d3acc2e1d1dc0f272e970)
  3. Looking forward to other contributions from you