Merge pull request #368 from reagent-project/class-fixes

Class fixes

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## Unreleased
+
+**[compare](https://github.com/reagent-project/reagent/compare/v0.8.0...master)**
+
+- Fix problem which caused using e.g. `:class` property with custom HTML element to break normal elements
+- Fix problem using keyword or symbol as `:class` together with element tag class shorthand, e.g. `[:p.a {:class :b}]` ([#367](https://github.com/reagent-project/reagent/issues/367))
+- Added support for using keywords and symbols in `:class` collection
+
 ## 0.8.0 (2018-04-19)
 
 **[compare](https://github.com/reagent-project/reagent/compare/v0.8.0-rc1...v0.8.0)**

project.clj

@@ -47,8 +47,8 @@
   ;; In future :main alone should be enough to find entry file.
   :cljsbuild
   {:builds
-   {:client
+   [{:id "client"
-    {:source-paths ["demo"]
+     :source-paths ["demo"]
      :watch-paths ["src" "demo" "test"]
      :figwheel true
      :compiler {:parallel-build true
@@ -59,8 +59,8 @@
                 :asset-path "js/out"
                 :npm-deps false}}
 
-    :client-npm
+    {:id "client-npm"
-    {:source-paths ["demo"]
+     :source-paths ["demo"]
      :watch-paths ["src" "demo" "test"]
      :figwheel true
      :compiler {:parallel-build true
@@ -70,8 +70,8 @@
                 :output-to "target/cljsbuild/client-npm/public/js/main.js"
                 :asset-path "js/out"}}
 
-    :test
+    {:id "test"
-    {:source-paths ["test"]
+     :source-paths ["test"]
      :compiler {:parallel-build true
                 :optimizations :none
                 :main "reagenttest.runtests"
@@ -81,8 +81,8 @@
                 :npm-deps false
                 :aot-cache true}}
 
-    :test-npm
+    {:id "test-npm"
-    {:source-paths ["test"]
+     :source-paths ["test"]
      :compiler {:parallel-build true
                 :optimizations :none
                 :main "reagenttest.runtests"
@@ -93,16 +93,16 @@
 
     ;; Separate source-path as this namespace uses Node built-in modules which
     ;; aren't available for other targets, and would break other builds.
-    :prerender
+    {:id "prerender"
-    {:source-paths ["prerender"]
+     :source-paths ["prerender"]
      :compiler {:main "sitetools.prerender"
                 :target :nodejs
                 :output-dir "target/cljsbuild/prerender/out"
                 :output-to "target/cljsbuild/prerender/main.js"
                 :aot-cache true}}
 
-    :node-test
+    {:id "node-test"
-    {:source-paths ["test/reagenttest/runtests.cljs"]
+     :source-paths ["test/reagenttest/runtests.cljs"]
      :watch-paths ["src" "test"]
      :compiler {:main "reagenttest.runtests"
                 :target :nodejs
@@ -113,8 +113,8 @@
                 :npm-deps false
                 :aot-cache true}}
 
-    :node-test-npm
+    {:id "node-test-npm"
-    {:source-paths ["test/reagenttest/runtests.cljs"]
+     :source-paths ["test/reagenttest/runtests.cljs"]
      :watch-paths ["src" "test"]
      :compiler {:main "reagenttest.runtests"
                 :target :nodejs
@@ -126,8 +126,8 @@
 
     ;; With :advanched source-paths doesn't matter that much as
     ;; Cljs compiler will only read :main file.
-    :prod
+    {:id "prod"
-    {:source-paths ["demo"]
+     :source-paths ["demo"]
      :compiler {:main "reagentdemo.prod"
                 :optimizations :advanced
                 :elide-asserts true
@@ -139,8 +139,8 @@
                 :npm-deps false
                 :aot-cache true}}
 
-    :prod-npm
+    {:id "prod-npm"
-    {:source-paths ["demo"]
+     :source-paths ["demo"]
      :compiler {:main "reagentdemo.prod"
                 :optimizations :advanced
                 :elide-asserts true
@@ -151,8 +151,8 @@
                 :closure-warnings {:global-this :off}
                 :aot-cache true}}
 
-    :prod-test
+    {:id "prod-test"
-    {:source-paths ["test"]
+     :source-paths ["test"]
      :compiler {:main "reagenttest.runtests"
                 :optimizations :advanced
                 :elide-asserts true
@@ -163,8 +163,8 @@
                 :npm-deps false
                 :aot-cache true}}
 
-    :prod-test-npm
+    {:id "prod-test-npm"
-    {:source-paths ["test"]
+     :source-paths ["test"]
      :compiler {:main "reagenttest.runtests"
                 :optimizations :advanced
                 :elide-asserts true
@@ -173,4 +173,4 @@
                 :output-to "target/cljsbuild/prod-test-npm/main.js"
                 :output-dir "target/cljsbuild/prod-test-npm/out"
                 :closure-warnings {:global-this :off}
-                :aot-cache true}}}})
+                :aot-cache true}}]})

src/reagent/impl/template.cljs

@@ -82,7 +82,7 @@
   (if (named? k)
     (if-some [k' (cache-get custom-prop-name-cache (name k))]
       k'
-      (aset prop-name-cache (name k)
+      (aset custom-prop-name-cache (name k)
             (util/dash-to-camel k)))
     k))
 
@@ -122,12 +122,18 @@
       ;; Merge classes
       class
       (assoc :class (let [old-class (:class props)]
-                      (if (nil? old-class) class (str class " " old-class)))))))
+                      (if (nil? old-class) class (str class " " (if (named? old-class)
+                                                                  (name old-class)
+                                                                  old-class))))))))
 
 (defn stringify-class [{:keys [class] :as props}]
   (if (coll? class)
     (->> class
-         (filter identity)
+         (keep (fn [c]
+                 (if c
+                   (if (named? c)
+                     (name c)
+                     c))))
          (string/join " ")
          (assoc props :class))
     props))

test/reagent/impl/template_test.cljs

@@ -0,0 +1,20 @@
+(ns reagent.impl.template-test
+  (:require [clojure.test :as t :refer [deftest is testing]]
+            [reagent.impl.template :as tmpl]
+            [goog.object :as gobj]))
+
+(deftest cached-prop-name
+  (is (= "className"
+         (tmpl/cached-prop-name :class))))
+
+(deftest cached-custom-prop-name
+  (is (= "class"
+         (tmpl/cached-custom-prop-name :class))))
+
+(deftest convert-props-test
+  (is (gobj/equals #js {:className "a"}
+                   (tmpl/convert-props {:class "a"} #js {:id nil :custom false})))
+  (is (gobj/equals #js {:class "a"}
+                   (tmpl/convert-props {:class "a"} #js {:id nil :custom true})))
+  (is (gobj/equals #js {:className "a b" :id "a"}
+                   (tmpl/convert-props {:class "b"} #js {:id "a" :class "a" :custom false}))))

test/reagenttest/runtests.cljs

@@ -7,6 +7,7 @@
             [reagenttest.testtrack]
             [reagenttest.testwithlet]
             [reagenttest.testwrap]
+            [reagent.impl.template-test]
             [cljs.test :as test]
             [doo.runner :as doo :include-macros true]
             [reagent.core :as r]
@@ -21,8 +22,7 @@
                      :color :#aaa})
 
 (defn all-tests []
-  #_(test/run-tests 'reagenttest.testratomasync)
+  (test/run-all-tests #"(reagenttest\.test.*|reagent\..*-test)"))
-  (test/run-all-tests #"reagenttest.test.*"))
 
 (defmethod test/report [::test/default :summary] [m]
   ;; ClojureScript 2814 doesn't return anything from run-tests
@@ -56,4 +56,4 @@
     (run-tests)
     [#'test-output-mini]))
 
-(doo/doo-all-tests #"reagenttest.test.*")
+(doo/doo-all-tests #"(reagenttest\.test.*|reagent\..*-test)")

test/reagenttest/testreagent.cljs

@@ -574,6 +574,27 @@
   (is (= (rstr [:p {:class #{"a" "b" "c"}}])
          (rstr [:p {:class "a b c"}]))))
 
+(deftest class-different-types
+  (testing "named values are supported"
+    (is (= (rstr [:p {:class :a}])
+           (rstr [:p {:class "a"}])))
+    (is (= (rstr [:p.a {:class :b}])
+           (rstr [:p {:class "a b"}])))
+    (is (= (rstr [:p.a {:class 'b}])
+           (rstr [:p {:class "a b"}])))
+    (is (= (rstr [:p {:class [:a :b]}])
+           (rstr [:p {:class "a b"}])))
+    (is (= (rstr [:p {:class ['a :b]}])
+           (rstr [:p {:class "a b"}]))))
+
+  (testing "non-named values like numbers"
+    (is (= (rstr [:p {:class [1 :b]}])
+           (rstr [:p {:class "1 b"}]))))
+
+  (testing "falsey values are filtered from collections"
+    (is (= (rstr [:p {:class [:a :b false nil]}])
+           (rstr [:p {:class "a b"}])))) )
+
 (deftest test-force-update
   (let [v (atom {:v1 0
                  :v2 0})