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 headless prop to remove default classes #248

Closed
wants to merge 2 commits into from

Conversation

stepanorda
Copy link

Closes #247.

Summary of major changes

  1. adds headless prop to remove default classes

Todos

  1. Not sure about other classes. If we were to remove them also, we need to add a ton of props to be able to replace them. I think this is good enough for now and will increase flexibility without braking anything for existing users.

Checklist

[] has tests (only needed if any new functionality was added or bugs fixed)
[
] has examples/docs (only needed if any new functionality was added)

@janosh
Copy link
Owner

janosh commented Jul 6, 2023

I think what people usually expect from headless is something that looks bland and generic but still has the right layout. Currently if you set headless={true}, it completely breaks the layout. Is that your intended behavior?

Perhaps we should be more surgical about which styles we disable in headless mode.

Screen.Recording.2023-07-06.at.3.15.53.PM.mp4

@janosh janosh added enhancement New feature or request style Anything CSS related discussion Gathering feedback on open questions labels Jul 6, 2023
@stepanorda
Copy link
Author

Well yes, that's intended behaviour. If you really want to style it with the tailwind(or other similar framework), usually that does mean that you NEED to style it. But I see options here:

  • Allow disabling some, not all classes
  • As you said, split layout and styling (but then you still end up having some conflicts and lose flexibility)
  • Maybe provide an example tailwind template in docs for people to start with

@janosh
Copy link
Owner

janosh commented Jul 7, 2023

As you said, split layout and styling (but then you still end up having some conflicts and lose flexibility)

Separating layout and design CSS is probably a bit of work but actually seems like a really good idea to me. If we manage that, we could even consider making the unstyled version the default. In that case, we should rename the prop to, say,

ui: 'styled' | headless' | 'bare' = 'styled'

where styled is the current look, headless would only contain the layout CSS and bare is the current result of headless=true shown in the video above.

Maybe provide an example tailwind template in docs for people to start with

I don't use tailwind myself but if you'd like to contribute such an example, that'd be much appreciated! 👍

@stepanorda
Copy link
Author

Even better might be to make a base headless component and decorator components with styles.
That way styles wouldn't be included in the bundle if you don't need them.
It would also would split the code and it's responsibilities better (visual and logic).

@janosh
Copy link
Owner

janosh commented Jul 7, 2023

Even better might be to make a base headless component and decorator components with styles.
That way styles wouldn't be included in the bundle if you don't need them.

Hmm... not sure. I expect the savings from excluding design CSS to be minimal. On the other hand, IIRC the Svelte Compiler has difficulty optimizing {...$$props} and {...$$restProps} so unless you want to list each prop explicitly in the styled wrapper component (which would be a lot of boilerplate), the runtime loss is probably bigger than the CSS savings.

But I'm happy to be convinced otherwise on both points.

@stepanorda
Copy link
Author

@janosh makes sense. Also, it would be a huge rewrite to do it that way. Maybe even easier to make headless from scratch. I think for now adding example tailwind classes and keeping a "headless" prop would be good enough for most use cases.

If you agree I will add example tailwind classes for people to start from.

@janosh
Copy link
Owner

janosh commented Jul 10, 2023

Sure, go ahead.

@stepanorda
Copy link
Author

Turns out that to make it work properly with a tailwind, it needs changes to the HTML structure. Also there are over CSS that doesn't have props for it. I guess this is all starting to be too complicated and not worth it. To make something headless it needs to be designed that way from the beginning. I think we can mark this as a failed experiment and close this PR.

@stepanorda stepanorda closed this Jul 11, 2023
@janosh
Copy link
Owner

janosh commented Jul 11, 2023

No worries. Thanks for documenting your conclusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Gathering feedback on open questions enhancement New feature or request style Anything CSS related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using class props built in classes has higher priority
2 participants