Skip to content
This repository has been archived by the owner on Sep 16, 2022. It is now read-only.

Explore 'cloning' children #19

Closed
joe-bell opened this issue Apr 26, 2020 · 9 comments
Closed

Explore 'cloning' children #19

joe-bell opened this issue Apr 26, 2020 · 9 comments

Comments

@joe-bell
Copy link
Owner

Having used raam in a few side projects, I feel like I'm missing the markup flexibility.

I'd like to look at a cloneElement variant where styles are appended to children rather than wrapped.

@robphoenix
Copy link

Yes, I found this as well when using the Braid approach of wrapping the children. I experimented with cloning children - system-ui/theme-ui#809 (comment) (there were some issues when using emotion), but reverted it in the end, and I'm really annoyed that I can't remember exactly why, sorry, could well have been some silly restriction in the work component lib.

Currently I've ended up copying the react-ui approach, using the lobotomized owl selector.

@joe-bell
Copy link
Owner Author

joe-bell commented May 7, 2020

My go-to approach was always the lobotomized owl, but the problem is that every component in theme-ui is class-based which means we need to double the specificty to override margins

With this I'm keen to extend the cloned element's styles rather than override, if that makes sense?

@robphoenix
Copy link

It didn’t at first, but yes, that does make sense. Trying to build these reusable layout components is massively helping my CSS understanding!

@jparklev
Copy link

jparklev commented Jun 13, 2020

I've been thinking of using this libaray, and this would be great imo! In a project where i'm using theme-ui, i've been doing

import { jsx } from 'theme-ui';
import { css } from '@theme-ui/css';

React.Children.map(children, child => {
      const style = css({
        mb: gap
      });

      return 'css' in child.props
        ? React.cloneElement(child, {
            css: theme => [child.props.css(theme), style(theme)]
          })
        : jsx(child.type, { .key: child.key, ...child.props, css: style });
})

It doesn't feel like the cleanest solution, but, based on one of the emotion maintainer's comments here, maybe it's fine. Do you think something like this could work in raam's case as well?

@joe-bell
Copy link
Owner Author

Thanks @jparklev, this is super helpful! I will give this approach a try soon and keep you posted

@joe-bell
Copy link
Owner Author

joe-bell commented Jul 5, 2020

Hey @robphoenix and @jparklev!

I've been thinking about this more over the weekend and wanted to run an idea by you both. Rather than "cloning" the element, what if instead there was the option to use an Item element of the layout?

So for example, with Stack:

<Stack>
  <StackItem as="p">
    Hello I'm a paragraph
  </StackItem>
</Stack>

I'm still playing around with ideas, but I feel like something like this could be significantly less messy to manage and could be kept alongisde the current API without introducing breaking changes

@jparklev
Copy link

jparklev commented Jul 5, 2020

Hmm yea, i think – in combination with the current API – this could be good. My gut feeling is it would be preferable not to add a new element (and avoid somebody new looking at your frontend code and having to ask why sometimes Stack has StackItems and sometimes not). However, I certainly agree with this being "significantly less messy," even more so the longer I use the cloning approach in my own projects

@joe-bell
Copy link
Owner Author

joe-bell commented Jul 6, 2020

I've looked into this some more and I think I'm going to drop this now…

I tried an implementation where I used a context/provider but it becomes difficult to check the "type" of the children (and knowing when to apply styles), especially in Emotion

I like the idea of having this as a feature, but I'm kinda thinking for odd use-cases it's best to create a use-case style.

If either of you have the time to look into this again in the future, please feel free to re-open!

@joe-bell joe-bell closed this as completed Jul 6, 2020
@robphoenix
Copy link

it becomes difficult to check the "type" of the children (and knowing when to apply styles)

Yeah I found this recently, you can't guarantee your children are going to be StackItems. Which can be ok, reach-ui does a lot of composition where a certain weight of responsibility is on the user to use the right components. However for a library such as this which is standalone layout components, rather than contained as a part of a larger system, I guess you have to expect users to put whatever they want in the Stack and for the Stack to deal with it all appropriately.

FWIW I've ended up straight up copying Braid's Stack and so far it's given me no issues at all, whereas all other options I've tried have had edge case issues. The only thing I dislike about it is all the extra divs, but such is life 🤷

This was referenced Oct 6, 2020
@joe-bell joe-bell mentioned this issue Feb 18, 2021
23 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants