germsvel / phoenix_test

MIT License
144 stars 20 forks source link

Should non-text fields support corresponding types? #98

Closed soundmonster closed 1 month ago

soundmonster commented 1 month ago

For input fields other than "text", should PhoenixTest support the corresponding data type?

session
# this works
|> fill_in("Number input", with: "10")

# this doesn't
|> fill_in("Number input", with: 10)
# ** (FunctionClauseError) no function clause matching in URI.encode_www_form/1
#
#     The following arguments were given to URI.encode_www_form/1:
#
#         # 1
#         10
#
#     Attempted function clauses (showing 1 out of 1):
#
#         def encode_www_form(string) when is_binary(string)

# analogously, this works
|> fill_in("Date input", with: "2023-12-30")
# while these don't
|> fill_in("Date input", with: Date.utc_today())
|> fill_in("Date input", with: Date.new!(2023, 12, 30))

A possible solution would be to add a type field to the Field struct and then have a check with possible conversion in Field.to_form_data/1.

I can draft a fix, the question is:

germsvel commented 1 month ago

That's a really interesting idea @soundmonster!

I think you add a good question:

what other input types will need special handling, besides number and date?

I honestly don't know. But I do think that it kind of makes sense for number inputs to accept... well numbers. And date inputs to accept dates.

I also think not accepting those types is somewhat surprising behavior. If we think of someone using the library for the first time, they would probably find it an odd surprise that you can't fill_in("Number input", with: 10).

So, I wonder if we could support this in a semi-simple way? Maybe we call to_string/1 on inputs? Is there some type of input for which that wouldn't work?

soundmonster commented 1 month ago

If we think of someone using the library for the first time, they would probably find it an odd surprise that you can't fill_in("Number input", with: 10).

It actually used to work before the move away from DeepMerge to Plug.Conn.Query, so there might even be examples in the wild that suggest that it works.

Maybe we call to_string/1 on inputs? Is there some type of input for which that wouldn't work?

I can't think of one. Right now, the only supported value type is string/binary.

I've drafted #100, please have a look