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

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

lud-hu
Copy link

@lud-hu lud-hu commented Apr 24, 2020

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).

@karltaylor
Copy link
Collaborator

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 😊

@lud-hu
Copy link
Author

lud-hu commented Jan 30, 2021

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.

@karltaylor
Copy link
Collaborator

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?

@lud-hu
Copy link
Author

lud-hu commented Jan 30, 2021

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.

@karltaylor
Copy link
Collaborator

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 :)

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.

triggerSibling Issue
2 participants