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

Inspector improvements for dynamic <Select> and <Menu> support. #292

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

Conversation

Azq2
Copy link

@Azq2 Azq2 commented Apr 26, 2024

Fixing issue #223.
This done by dynamic linking between HTML nodes and components.

Of course, this is a hacky and bad workaround.
But, I think, it is better than, broken <Select> and <Menu> which makes this awesome lib unusable in most cases. :(

@Azq2
Copy link
Author

Azq2 commented Apr 27, 2024

Demo with fix: https://github.com/Azq2/suid-select-bug

@BierDav
Copy link
Contributor

BierDav commented Apr 29, 2024

@Azq2 does this also fix the issues with ssr though?

@BierDav
Copy link
Contributor

BierDav commented Apr 29, 2024

Ok now, I've understood what you are doing here, but for my taste this is a bit too hacky. I would prefer a solution with an excessive use of contexts, because it also would solve the issue with ssr.

@Azq2
Copy link
Author

Azq2 commented Apr 29, 2024

Yes, you are right. This doesn't help with SSR and is too hacky.
But I positioned this fix as an intermediate solution before a better variant was implemented.

  • Working CSR is better than broken SSR+CSR.
  • This is only the fix of another existing workaround. The code won't get better or worse at all.

@BierDav
Copy link
Contributor

BierDav commented May 1, 2024

If you need a solution to get csr with for working, you may use my implmentation: https://github.com/BierDav/suid-extensions
You have to download the code and add it to your codebase. This btw. also supports SSR

@Azq2
Copy link
Author

Azq2 commented May 4, 2024

This PR is not for me, I have already fixed this problem in my project.

This PR is for people who first use this library.
They want a working library, not a lot of hours of debugging, because nowhere said about these problems.

And this PR is a first step to fixing that.
The next step - reimplementing all the similar code to using contexts instead of hacky inspect().


@juanrgm What you think about this?

I know that open-source is always “as is” and no one is obliged to anyone.
But I believe that we all have a responsibility for the future of front-end development.
And a broken library of this level will only repel people who decide to try SolidJS.

We must join effort to fix this.

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.

None yet

2 participants