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

Form values should return a Vec of values #2043

Closed
serzhiio opened this issue Mar 9, 2024 · 4 comments · Fixed by #2103
Closed

Form values should return a Vec of values #2043

serzhiio opened this issue Mar 9, 2024 · 4 comments · Fixed by #2103
Assignees
Labels
html Related to the html crate

Comments

@serzhiio
Copy link
Contributor

serzhiio commented Mar 9, 2024

Here i suggested more classic browser behavior #1834 , which was implemented in #1836.
Why was it changed again?

Let's assume we have group of fields with same name, in browser they are aggregated under same name in Array, like this:
field_name = ["value1","value2","value 3"]
even if one of values is empty it will persist in the array as empty value.
field_name = ["","","value 3"]

Current behavior is pushing all values into String with comma as separator:
field_name = "value1,value2,value3"

If one of values is empty it is simply excluded, but this is very tricky bc looks like there are no other fields:
field_name = "value1" - here looks like we have no more fields

And even this is possible:
field_name = ",value2,value3" - here first value is excluded, but the value exists but epmty

Снимок экрана от 2024-03-09 19-27-39

@serzhiio
Copy link
Contributor Author

serzhiio commented Mar 9, 2024

Everything i, personally, want is to have ability to understand how many fields with same name has been sent (even with empty values). Such values should not be united under one string.
This is also related to checkbox values, bc now if it is not checked(off) value not sent.

I believe that the right way is to send all values.

@serzhiio
Copy link
Contributor Author

serzhiio commented Mar 9, 2024

I am sorry guys, too much work for Saturday!
The only API change is that everything is inside String delimited with comma, it is not a enum anymore.
I can handle it.

@serzhiio serzhiio closed this as completed Mar 9, 2024
@jkelleyrtp
Copy link
Member

jkelleyrtp commented Mar 9, 2024

I was fixing forms in #1974 and one of the bugs was handling serialization with the array values. I'd like to keep a single type that we serialize across the border instead of being polymorphic over strings/arrays since they're quite similar.

Either we send only arrays or we send only strings. I chose comma-delimited strings - we might be missing an accessor that splits the string for you. The alternative is send only arrays and an accessor that gets the first value of that array. Either works.

The native browser type is an array I believe, so I think your implementation is closer to the browser.

Having an API as common as forms requiring an enum import and matching makes the user-facing API clunky, hence why it's just a string now. Plus, it's always better to have fewer types.

I recommend maybe pinning to a commit or 0.5.0-alpha.0 since we are merging breaking stuff that might not make it into the final release, given feedback (like this issue!).

@ealmloff
Copy link
Member

ealmloff commented Mar 11, 2024

I was fixing forms in #1974 and one of the bugs was handling serialization with the array values. I'd like to keep a single type that we serialize across the border instead of being polymorphic over strings/arrays since they're quite similar.

Either we send only arrays or we send only strings. I chose comma-delimited strings - we might be missing an accessor that splits the string for you. The alternative is send only arrays and an accessor that gets the first value of that array. Either works.

The native browser type is an array I believe, so I think your implementation is closer to the browser.

Having an API as common as forms requiring an enum import and matching makes the user-facing API clunky, hence why it's just a string now. Plus, it's always better to have fewer types.

I recommend maybe pinning to a commit or 0.5.0-alpha.0 since we are merging breaking stuff that might not make it into the final release, given feedback (like this issue!).

Merging the form values into a string loses information which makes it impossible to parse some forms like this:

select {
    multiple: true,
    oninput: move |ev| {
        println!("Input event: {:#?}", ev);
        println!("Values: {:#?}", ev.value().split(',').collect::<Vec<_>>());
    },
    // There is no way to know if the user has selected 1 and Item or Item,1
    option { value: "Item,1",  "1" }
    option { value: "1",  "2" }
    option { value: "Item",  "3" }
}

@jkelleyrtp jkelleyrtp reopened this Mar 11, 2024
@jkelleyrtp jkelleyrtp changed the title Form behavior changed again Form values should return a Vec of values Mar 11, 2024
@ealmloff ealmloff added the html Related to the html crate label Mar 11, 2024
@jkelleyrtp jkelleyrtp self-assigned this Mar 18, 2024
@jkelleyrtp jkelleyrtp added this to the 0.5.0: Signals milestone Mar 18, 2024
jkelleyrtp added a commit that referenced this issue Mar 19, 2024
Fix #2043: use formvalue instead of String for forms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
html Related to the html crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants