-
-
Notifications
You must be signed in to change notification settings - Fork 490
feat(solid-form): add createFormHook
for solid-js.
#1597
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
Conversation
View your CI Pipeline Execution ↗ for commit faf6ef3
☁️ Nx Cloud last updated this comment at |
Will start the work on docs after getting the green light on the PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1597 +/- ##
==========================================
+ Coverage 89.38% 89.55% +0.17%
==========================================
Files 33 34 +1
Lines 1460 1494 +34
Branches 368 370 +2
==========================================
+ Hits 1305 1338 +33
- Misses 138 139 +1
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The feature's already in the works over at #1453 . Would you mind taking a look at it and giving feedback? |
Looks like it has the same changes I've made, I've just addressed the comments there and used the types that were defined in the React implementation, I guess. We can change the target branch from main to that PR if we want to track it there. |
I'm using this PR in my project and fixing problems for now. |
afe9475
to
ebd3774
Compare
This fixes and issue where some parts of the form, like submitting didn't work.
@MAST1999 you are AWESOME. Thank you for taking this on! What can we do to support? |
Hey I think it's ready, I'm using it in my hobby project and it's working. But could always use a review or more testing. |
Awesome! In that case do you wanna start writing some docs for it? Even if it's just copy+pasted from the React one with Solid.js syntax? |
Sounds good Will get started! |
@MAST1999 I saw that you added docs, are we ready for final review and/or merge or are we waiting on something else? :) |
It's ready for final review. I wanted to add examples as well but I noticed that we use released versions for examples, So I'll open another PR with examples after release. |
Oh, actually, that's a great callout. Can we build out a We don't need to use the released version, Completely safe. |
Sorry for the continual last-minute requests 😅 |
Don't worry about it. This is what I wanted to do anyway 😁 |
OK, found and interesting problem that I hadn't run into. It happens when I try using the store on the I've added a comment on the Looked around, but I couldn't figure out where the problem is exactly. If you access the fields itself for value and errors it works correctly now, if you try using the |
Uhm... Wow.... Our Solid Store adapter is real broken. I'm a bit surprised we didn't run into problems before. Let me try to fix that |
Should be fixed now! Launching as soon as the CI passes! Thanks a million once again! |
Oh dear... I've seem to have merged too early. Much of the composition's docs are still in React. @MAST1999 could you update those for us when you get a chance? :) |
Yep, will send a PR soon |
I was starting a new project and decided to use solid, and I found out that
createHookForm
was missing so here I am 😄I don't have too much experience with solid, but I think this looks good.