react-collapsible icon indicating copy to clipboard operation
react-collapsible copied to clipboard

Fixes #99 and adds wrapper around trigger and "non clickable trigger" for styling possibility.

Open lud-hu opened this issue 4 years ago • 5 comments

Fixes #99 with the described solution (see ticket) and introduces a wrapper element (__header) so that it makes sense to have the nonClickableTriggerElement (otherwise its hard to style).

lud-hu avatar Apr 24 '20 12:04 lud-hu

Hey @lud-hu, I'm slightly reluctant to merge this with two underlying fixes. What was the issue that warrants a "wrapper element" that you mention? Will leave this open for a week or so just to clarify but if no response I'll close to keep repo clean 😊

karltaylor avatar Jan 30 '21 10:01 karltaylor

Hi @karltaylor,

OK this was quite some time ago but I can see my reasons for doing it like this again! 😅 The fix that you have proposed in the mentioned ticket allows to pass a react node as a triggerSibling. If you do this (and not just pass string) you'll probably want to style the header section so that the elements are shown where they are supposed to be. E.g. I've done it like this: image (The checkbox on the right is the trigger sibling. I can't be just a string and can't be triggering the collapse function.)

I hope this explains the reasons. I think it's quite a useful feature to have.

lud-hu avatar Jan 30 '21 10:01 lud-hu

Hey @lud-hu thanks for your explanation!

I get what you're saying now, it's not possible to style them in a flex way because there's no wrapping container and your <div className={headerClassString} /> fixes that.

I think moving forward it would be good to move away from adding extra classNames and perhaps let the user pass in a style prop for the specific element.

Maybe something like

<Collapsible triggerContainerStyle={{ flex: 1 }} />

I also quite like the way react-select allows you to customize components and think this would be a great approach https://react-select.com/components.

What do you think?

karltaylor avatar Jan 30 '21 11:01 karltaylor

Hi @karltaylor, Yes, this would also be a good approach! I like the react-select way of doing this as well. But currently react-collapsible is only using the class names, that's why I did it like this.

lud-hu avatar Jan 30 '21 11:01 lud-hu

Hi @karltaylor, Yes, this would also be a good approach! I like the react-select way of doing this as well. But currently react-collapsible is only using the class names, that's why I did it like this.

I'm wondering if it's possible to do it this way, on a conditional basis, because I worry adding another div into the component might break current implementation in the event it's been styled using element+element css selectors, like .Collapsible + div.

I'll keep thinking :)

karltaylor avatar Jan 30 '21 21:01 karltaylor