-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
x-model.fill by input value on null or empty string #3423
x-model.fill by input value on null or empty string #3423
Conversation
@Restartz thanks! We’ll continue the discussion here. Can you update the PR description with summary of what this PR is about in the state it’s currently in so people don’t need to dig through the discussion? Thanks! |
thanks @Restartz for doing this! |
No problem! |
@Restartz great, thanks for doing that! 😁 Regarding the failing test |
I will voice that I think any nullish value in data should be automatically replaced by the value of the input even without any modifier (just |
|
@agusprasetyo328 did you mean to make your whole comment a quote? I can't see your comments? |
@Restartz Caleb has said this looks good and let's stick with |
@joshhanley Added! Not sure if its thorough enough but it's something. 😅 |
@Restartz thanks! I ended up rewording it, hopefully what I've written makes sense 🙂 |
I see this was merged where fill only overrides A use case of providing a default in the data, and allowing an input to override it makes sense to me. |
@ekwoka hmm is there a scenario where an Alpine property is undefined? Yeah not sure whether fill should always overwrite or not. I can see benefits both ways. |
It could be initialized as At least that's my thoughts on it. |
Can this be update to work with selects? e.g. change: if (modifiers.includes('fill') && el.hasAttribute('value') && (getValue() === null || getValue() === '')) {
setValue(el.value)
} to: if (modifiers.includes('fill') && (getValue() === null || getValue() === '')) {
if (el.hasAttribute('value')) {
setValue(el.value)
} else if (el.tagName.toLowerCase() === 'select' && el.selectedIndex > 0) {
setValue(el.options[el.selectedIndex].value);
}
} Any reason why not? |
@ekwoka ah yeah. My thoughts would be that I wouldn't want an input dictating which props could be added to the component, just that they could use existing ones. So if there was a way to detect if the prop exists but is @manticorp I don't see any reason why it couldn't be updated to support them too. Could be useful to set the selected option on the select, so it's displayed correctly when the HTML is rendered, then once Alpine is booted it gets the value from the select. Saves needing to both output "selected" on the correct element and also pre-filling Alpine's prop with the same data. |
@manticorp I made #3495 so that it will work with select and other modifiers. Basically reworked it to just dispatch the same event that is being listened to for maximum simplicity, as opposed to directly updating the value. |
Nice one! Looks good 👍 thank you! |
The .fill is such a useful addition, but I struggled with figuring out its behavior with multiple select fields. I've started a new discussion on it here: #4135 and I wonder if it should be treated as a bug or if it's intended. |
Set an x-model bound value to the value of the input when the bound value is null or an empty string,
but as stated in the original discussion this might actually be expected default behaviour.
The implementation is currently using a "fill" modifier (name subject to change) for now, but might be better as default behaviour with a modifier for the possibility of overwriting any non-nullish value as well, as suggested by @ekwoka (#1985 (reply in thread))
Original request by @motine here: #1985
Example usage:
Result:
since
myValue
in the example is initiallynull
, thanks to the modifier it will be set to123456