phoenixframework / phoenix

Peace of mind from prototype to production
https://www.phoenixframework.org
MIT License
21.46k stars 2.88k forks source link

Update :text column to textarea to match intent for html generator #5963

Closed lifeiscontent closed 1 week ago

ShPakvel commented 2 weeks ago

Thank you for catch. 🙇🏼

SteffenDE commented 2 weeks ago

Hi @lifeiscontent,

I'm not sure if we can really assume that users that specify text as column type also want a textarea necessarily? In any case we'd need to have the same behavior in the phx.gen.live generator as well.

It is also really not a big change to do yourself after running the generator. Let's see what others think :)

ShPakvel commented 2 weeks ago

@lifeiscontent +1 from my side on similar update for live form. And also I suggest to update this test with additional text field, to check textarea input in this form assertion.

@SteffenDE, regarding choice between input and textarea. Usually type string associated with input in html, and type text associated with textarea in html, as text type chosen for databases usually means "we expect lot more words to store here". I believe it is general practical approach to map html-to-db this way.

lifeiscontent commented 2 weeks ago

@ShPakvel yep, my thoughts exactly. I'll try to follow up on this PR over the weekend. :)

josevalim commented 2 weeks ago

Yes, text is meant to be translated to textarea, otherwise use string. :+1:

lifeiscontent commented 2 weeks ago

alright, all updated, including tests! Thanks for the support guys :)

SteffenDE commented 2 weeks ago

@lifeiscontent I don't see phx.gen.live being updated?

see https://github.com/phoenixframework/phoenix/blob/dad45278246207b26aa0c42c02398106c10e2b19/lib/mix/tasks/phx.gen.live.ex#L287-L288

lifeiscontent commented 1 week ago

@SteffenDE I'm not sure what happened, but it appears my work went missing during tranmission, all fixed now 😅

lifeiscontent commented 1 week ago

@SteffenDE bump

SteffenDE commented 1 week ago

Thanks for bumping! 🙌🏻