`girouette` child selectors ignored

[Vynlar]

Summary

girouette rules that produce garden that targets child elements style the parent element rather than the children.

Example

GIven this ornament definition:

(o/defstyled my-component :div :space-y-2)

Expected CSS:

.my_app__my_component * > * {
  margin-top: "0.5rem";
}

Actual CSS:

.my_app__my_component {
  margin-top: "0.5rem";
}

Notice the missing * > *

Why this is happening

girouette returns a vector of [selector styles] for the :space-y-2 rule (see example here):

[#garden.selectors.CSSSelector{:selector ".space-y-2 > * + *"} {:margin-top "0.5rem"}]

On this line in ornament: https://github.com/lambdaisland/ornament/blob/main/src/lambdaisland/ornament.cljc#L275 we see:

 (second girouette-garden)

which is only grabbing the styles part of the girouette output (the map containing margin-top). This results in the element itself receiving the styles, rather than its children. This will affect any other girouette rules that target child elements as well.

Solution

It seems that we will need to examine the first element in the garden vector returned from girouette to determine if it is a simple class name or a more complex selector that targets children.

With some guidance, I am willing to work on a PR to address this issue.

[green-coder]

If it can help, I am willing to modify Girouette to make it easier to reuse the Garden data it outputs. Just let me know what you need, or (even better) provide a PR.

[plexus]

Hi Vynlar, thanks for the report! I'd be very happy to take you up on your offer.

I suggest you start adding one or more test cases, so that we can unambiguously agree on the expected behavior, and have an easy way to verify the fix. I think we do have tests for the base case (Girouette styles that affect the component itself), but if we don't or they don't seem adequate you could add an extra one, so that we also know the common case doesn't break.

It seems like in this case we'd actually have to parse or otherwise process that CSSSelector to strip out the .space-y-2. I guess it's just a matter of string processing looking for whitespace after the first token, but perhaps we could be helped there by Girouette representing this as a nested selector, so that the old logic (naively calling second) actually still works. @green-coder do you think that would be an easy change?

;; pseudocode
[:.space-y-2 [(css-selector "* < *") ...]]
;; or
[:.space-y-2 [:* [:< [:* ...]]]]

In that case we wouldn't need a "fix" in Ornament at all, but a test case would still be very useful.

[plexus]

Great bug report by the way! If all issues people filed would contain this much context my life would be a lot easier :)

[Vynlar]

@plexus I like the idea of modifying girouette to push the * > * selector one level deeper. 👍 That seems like the easiest approach.

I have one reservation: If we want to ensure this doesn't break again in the future, could we make it a guarantee that the first element of the garden data returned by girouette is only the classname? Perhaps this discussion should be moved over to the girouette repo.

[plexus]

seems this got addressed in Girouette? @Vynlar if you still have the time a PR with a test case and a version bump for Girouette would be much appreciated. If not I'll just bump the version and make a new release.

[Vynlar]

@plexus I'll be happy to add the test case and bump the version, but I'll need @green-coder to create a new release and push it up to clojars. Do you think you'll have an opportunity to do that, @green-coder ?

[plexus]

Oops I misinterpreted the last activity on that PR, it looked like it had been released.

[green-coder]

I will make the release this week end.

[green-coder]

Girouette v0.0.6 is released.

[Vynlar]

I've opened up a PR to bump the version and add a test for the issue discussed here :)

Note: I was unable to get the cljs tests to run locally on my machine. I believe it has to do with my environment. I have confirmed the clj tests pass.

[plexus]

Thanks @green-coder! I'll see what's up with the cljs tests @Vynlar, thanks a lot!

[plexus]

This should be fixed in

[com.lambdaisland/ornament "0.2.19"]
{com.lambdaisland/ornament {:mvn/version "0.2.19"}}