-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fixed issue where initCheck : false was not working #22 #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for the part of updating storeValue
right after defining it (maybe I don't fully understand that part tho, reactivity and such :D)
src/index.js
Outdated
@@ -141,6 +142,8 @@ export function form(fn, config = {}) { | |||
config | |||
); | |||
|
|||
storeValue.update(value => ({ ...value, initCheck: config.initCheck })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something or could this not just be added to `storeValue` right away 🤔 (so move config
above const storeValue = ...
(L128)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be done @AnthyG , I did not want to move the structure much, that's why I haven't moved it.
} else { | ||
node.classList.remove(valid); | ||
node.classList.add(invalid); | ||
if ((!context.initCheck && field.dirty) || context.initCheck) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@AnthyG Should I make your suggested changes and push again? |
Yeah, if there hasn't been any specific reason why you used It keeps the code cleaner in the long run ;) |
@AnthyG and @chainlist I have updated the code fixing the issue(#22) for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@DhyeyMoliya thanks for the changes! |
Fixes Issue : #22
Description : Used
field.dirty
andconfig.initCheck
to control css classadd/remove