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

Send JS command values on phx-submit #3688

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

Send JS command values on phx-submit #3688

wants to merge 1 commit into from

Conversation

SteffenDE
Copy link
Collaborator

Alternative implementation for #3674.

Alternative implementation for #3674.

Co-Authored-By: João Otávio Biondo <[email protected]>
this.pushWithReply(refGenerator, "event", {
type: "form",
event: phxEvent,
value: formData,
meta: meta,

Choose a reason for hiding this comment

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

For the phx-value-* and JS.push/2 :value attributes, should we put inside a value or values key in meta?

Example:

<form phx-submit={JS.push("...", value: %{js_push_value: 1})} phx-value-attribute="2"></form>

# would have this meta:

meta: {
  _target: ...,
  js_push_value: 1,
  attribute: "2"
}

# but maybe it should be

meta: {
  _target: ...,
  value: {
    js_push_value: 1,
    attribute: "2"
  }
}

To not mix these values with possibly other meta keys that the submit might have.
What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it matters, because to keep the current behavior, either way values could be overridden somewhere.

E.g. you could do JS.push("..."; value: %{_target: "other target"}) and even with this change, on the server when we merge the values, the "other target" would win.

Choose a reason for hiding this comment

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

Hmmm got it!

so for me it looks good! :)

@SteffenDE SteffenDE requested a review from josevalim March 6, 2025 20:00
Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

LGTM we just need to make sure we are consistent. Who wins between JS.push, phx-value and the form inputs? We need to make sure this is documented too.

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.

3 participants