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

Only munge props when converting to native React elements #40

Merged
merged 9 commits into from
Mar 25, 2019

Conversation

lilactown
Copy link
Collaborator

This PR changes the way that hx.hiccup converts props when parsing hiccup, so that it will only do camel->kebab and other munging if the element being passed the props is a keyword (e.g. :div, :span).

It also changes the way hx.react/defnc parses the props object, so that it will not do camel->kebab and other munging.

The following code exemplifies the new behavior:

(defnc Foo [{:keys [text-to-render]}]
  [:div {:style {:color "red"}} "Foo: " text-to-render])

(hx/f [Foo {:text-to-render "This text shows"}])

(hx/f [Foo {:textToRender "This text doesn't show"}])

(defnc Bar [{:keys [textToRender]}]
  [:div {:style {:color "blue"}} "Bar: " textToRender])

(hx/f [Bar {:text-to-render "This text doesn't show"}])

(hx/f [Bar {:textToRender "This text shows"}])

:class is special-cased to be made available as :class-name for backwards compatibility reasons.

@orestis
Copy link
Member

orestis commented Mar 19, 2019

I just tried this out with some react-bootstrap components we are using, and it's slightly broken...

It seems like this change doesn't handle converting :class, :style and the other special properties of React to :className, :style with camel-cased names.

So for example:

[bootstrap/Form.Check {:label "Have never logged in"
                              :type "radio"
                              :checked (current-selection :have-never-logged-in)
                              :onChange (on-change :have-never-logged-in)
                              :class form-check-class
                              :id (unique-id 3)}]

yields the warning

Warning: Invalid DOM property `class`. Did you mean `className`?

@lilactown
Copy link
Collaborator Author

That sounds correct - I removed all of the props conversions for Components as elements. For interop purposes it would probably be good to keep around className and style conversions, you're right.

@orestis
Copy link
Member

orestis commented Mar 19, 2019

From a quick glance at the React reference (and at the existing source code), it seems like className (class), and style and potentially htmlFor are the only ones that we need to convert.

There's a bunch of others too, but I don't think we need to worry about them.

There's also the case of fully custom attributes, which React expects to be full lowercase (e.g. x-placement or data-something). I don't think we need to do anything there, just pass the original name in converted into a string and be done with it.

@lilactown
Copy link
Collaborator Author

I have added conversion of :class to "className" and :for to "htmlFor", as well as conversion of styles prop to JS, back to props conversion for functions/classes.

@lilactown lilactown merged commit 3747200 into master Mar 25, 2019
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