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

🐛 Makes .fill work with selects and other modifiers #3495

Merged
merged 1 commit into from May 10, 2023

Conversation

ekwoka
Copy link
Contributor

@ekwoka ekwoka commented Mar 30, 2023

This PR extends #3423 to support select elements and other modifier like number

@saveman71
Copy link

I'm just chiming in, do you think textareas support could be added to this PR?

#3547

@SimoTod
Copy link
Collaborator

SimoTod commented Apr 26, 2023

This PR is already covering that since it just triggers the event like a normal x-model (the current tagged implementation check for the existence of the value attribute which doesn't work for textareas).

@ekwoka
Copy link
Contributor Author

ekwoka commented Apr 27, 2023

@saveman71 This PR should handle everything model works on, as it essentially taps in to the existing model handling to do the filling, as opposed to doing special paths or exceptions.

So anything model works on, should now work with .fill if/when this PR is released.

@saveman71
Copy link

Very clear! I hadn't had a look at the code sorry but this implemention indeed seems more robust. Thanks a lot! Let's hope it gets merged :)

@@ -87,7 +87,10 @@ function injectHtmlAndBootAlpine(cy, templateAndPotentiallyScripts, callback, pa
})
}

export let haveData = (key, value) => ([ el ]) => expect(root(el)._x_dataStack[0][key]).to.equal(value)
export let haveData =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, just a heads up, making non-code-changing, styling changes (especially to styles that diverge from the rest of the codebase), makes PR's harder for me to review and merge without putting extra work in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On this note, would you be able to add the workspace settings/formatter settings you use to the repository?

Then it becomes automatic that anyone cloning the repo would have the same formatter.

@calebporzio calebporzio merged commit 61afc45 into alpinejs:main May 10, 2023
1 check passed
@noniq
Copy link

noniq commented May 15, 2023

I tried using .fill with multiple checkboxes bound to an array, but it does not seem to work. As far as I understand this is because for multiple checkboxes the initial (“empty”) value would need to be [] instead of null or "".

Do you think it would work to just add [] to the values checked in https://github.com/ekwoka/alpine/blob/bedbfc918cd0e3f9158d1216fc8e332a360cc81d/packages/alpinejs/src/directives/x-model.js#L73 ? Should I prepare a PR for that?

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

5 participants