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

Make FormControlLabel.tsx ssr compatible #254

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Conversation

BierDav
Copy link
Contributor

@BierDav BierDav commented Aug 20, 2023

This potentially makes old code a bit more inefficent when using typography directly as label, but sadly there is no way of reliably checking what the isTypography test for, because we have just access to an html string, which would be even more inefficent to parse it as just wrapping everything in a div.

And when someone wants to be as efficent they have to just set the disableTypography prop. In my opinion it's a worth tradeoff.

@juanrgm
Copy link
Member

juanrgm commented Aug 28, 2023

Why don't you use isServer?

  const isTypography = (v: unknown) =>
    !isServer &&
    v instanceof HTMLElement &&
    v.classList.contains(Typography.toString());

@BierDav
Copy link
Contributor Author

BierDav commented Aug 28, 2023

That would make the whole pr useless, because it would lead to hydration errors

@BierDav
Copy link
Contributor Author

BierDav commented Aug 28, 2023

The only way we would be able to achieve this is by using a context just for this, but I don't think that is worth the effort. The majority is not using a separate typography because there aren't a lot of use cases where it is the best way to do so. But maybe in future we will get features of solid start that allow us to access the already rendered html more easily.

@juanrgm juanrgm merged commit 69728b0 into swordev:main Sep 5, 2023
@juanrgm
Copy link
Member

juanrgm commented Sep 8, 2023

I reverted your commit (c62830e) and implemented a more suitable solution (here) compatible with SSR.

@BierDav BierDav deleted the pr-1 branch September 9, 2023 05:16
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