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

Add :caret slot with ability to customize its classes #87

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sax
Copy link

@sax sax commented Oct 29, 2024

No description provided.

@maxmarcon
Copy link
Owner

Hello @sax and thanks for the contribution! I have a question and a couple of suggestions:

Question: why is this slot attribute a list?

Suggestions:

  1. It would be nice to have a default implementation of the caret, turned off by default but that people can turn on if needed
  2. Styling of the caret should follow the styling conventions of the rest of the library, so I would add a caret_class and caret_extra_class options like for the other elements.
  3. There need to be tests that check that you can override the caret slot and set the styling attributes for it. The former tests are in live_select_test.exs and call the LiveComponent in live_component_form, to which the caret slot will have to be added. The latter tests live in component_test.exs.

Let me know if you need any help or have questions! Thanks

@sax
Copy link
Author

sax commented Nov 8, 2024

Sorry for the delay. I set the :class attribute on the slot to be a list because heex rendering treats attributes as iodatas rather than strings. Maybe there's a way to declare that, so people can use either strings or iodates.

Yeah, I was having issues understanding the test setup. Thanks for pointing me in a direction.

I have a set of tailwind classes that can be used as a default, but I am unclear what a daisy UI version would be.

@maxmarcon
Copy link
Owner

I have a set of tailwind classes that can be used as a default, but I am unclear what a daisy UI version would be.

daisyUI uses tailwind so very likely you could reuse the same classes

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