implements input_* form elements

[alexandrubagu]

I've implemented all input_* form elements @msaraiva @zamith

[msaraiva]

Hi @alexandrubagu!

Awesome job 🎉

I have a question for discussion before we merge this. As far as I could see, some of the inputs have the same properties and generate the same <input...>. The only difference is the type. Do you think it would be better to have a single component for those cases with a type property?, e.g. <Input type="date"/>, etc? Or do you feel that each one of them deserves its own component?

For the record, I'm not sure yet which approach is better so I'm trying to see the pros and cons :)

@zamith feel free to share your thoughts too.

[zamith]

Other than the comments I left, I tend to agree. We can probably abstract all of these into one Input component with a type. It will be a lot easier to maintain and the bulk of the work is already done by Phoenix.HTML.Form anyway.

We can (maybe?) still add a couple of wrappers over Input for ergonomics. For example RangeInput here has a min and max properties, which seems useful, so you don't have to send it via opts.

[msaraiva]

Regarding "creating own component" vs "using a generic input". One advantage of the current implementation (having one component for each function) is that we end up with a 1:1 mapping between them, which may be more intuitive for users.

I've just noticed we've missed events for both, the inputs and the form. I believe for the inputs we have at least phx-blur, phx-focus, phx-click, phx-capture-click, phx-keydown, phx-keyup and for the form, phx-change and phx-submit.

I guess we should define them without the phx- prefix, like:

@doc "Triggered when the component loses focus"
property blur, :event

@doc "Triggered when the component receives focus"
property focus, :event

Note: It's important that we add the events explicitly, otherwise, Surface will not set the phx-target automatically and the default target will be the parent live view.

[zamith]

Agree. It was in my plans to add those. As for the 1:1 mapping, I don't think there is much difference in terms of being intuitive from:

<TextInput ...>
<PasswordInput ...>

to

<Input type="text" ...>
<Input type="password" ...>

It both makes it "simpler" in terms of maintainability for us and more closely resembles actual HTML. That being said I don't have a strong preference either way, the main reason I'm leaning towards a generic component is having less code to deal with (E.g. we want to add the events, it would be changes to one file, instead of 10)

[alexandrubagu]

@msaraiva @zamith I've updated the PR but I don't know how to proceed further. From my point of view I prefer to have them in separate components as they are defined in Phoenix.HTML.Form.

Another idea is to have Surface.Components.Form.GenericInput and we could use metaprogramming to generate modules with the body from GenericInput for those input elements

[msaraiva]

Hi @alexandrubagu!

Thank you so much for taking the time to work on this. I think we could start creating a generic <Input> component with a type property. We could also define the valid values like:

property type, :string, values: ["hidden", "dete", "time", ...]

if we do this we can statically validate them in the near future and editors could also use that information to provide autocomplete/suggestions.

We can also keep specific components for any of them that we feel might bring better UX, like the RangeInput. No need to remove it.

Then we can wait for some feedback to see if we need to create the others.

The others should be created on top of the generic one. That would be a perfect scenario to test the :props directive and the import_props, which have not been implemented yet but are on the roadmap.

WDYT?

[msaraiva]

You shouldn't need warn: false here since Form is being used in context get. In case you remove it and start receiving warnings, please let me know, it's a bug.

[alexandrubagu]

In tests I've tried to remove warn: false and I got some warnings, so I kept them in place

[msaraiva]

@alexandrubagu I took a look at the specification of input at https://www.w3schools.com/tags/tag_input.asp

It looks like some the attributes are only applicable to some of the types. For instance, an input of type date might have attributes step, min and max but no placeholder nor maxLength. An input of type text has placeholder and maxLength but no step, min or max. That means having one component per type would give us the opportunity to improve UX later by adding those properties explicitly.

So let's move on with separate components. Thanks a lot, @alexandrubagu for your work and sorry for coming back and forth on this.

[alexandrubagu]

@msaraiva no problem.

I've updated the PR according to discussion from #82

[msaraiva]

@alexandrubagu thanks! ❤️