purescript-react / purescript-react-basic

An opinionated set of bindings to the React library, optimizing for the most basic use cases
https://pursuit.purescript.org/packages/purescript-react-basic
Apache License 2.0
281 stars 40 forks source link

`autoComplete` is `Boolean` but should be `String` on `input` #118

Closed i-am-the-slime closed 4 years ago

i-am-the-slime commented 4 years ago

So it can accept beautiful valid strings like: "false". 😁

megamaddu commented 4 years ago

Is this true of all autoComplete attributes or just input (or even specific input types)?

tesujimath commented 4 years ago

I don't understand why you want to pass a Boolean as a string. It really is a Boolean, right?

hdgarrood commented 4 years ago

Apparently it is not a Boolean, it's a space-separated list of the values described here: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/autocomplete

tesujimath commented 4 years ago

Ah, thanks! To be typesafe then, we'd want a new type AutofillDetailToken, with values corresponding to those strings, and pass in a list of these, or on, or off. Is this worth it?

i-am-the-slime commented 4 years ago

To be quite honest, I just used this attribute and react complained. When I unsafeCoerced "false" it took it. I'd go with what @hdgarrood says. Interesting that "false" isn't even one of the valid ones.

hdgarrood commented 4 years ago

I don't think a type like data AutofillDetailToken = On | Off | Name | [...] etc. is desirable, because adding a new constructor to an existing data type is always a breaking change. I would recommend something like

newtype AutofillDetail = AutofillDetail String

on :: AutofillDetail
on = AutofillDetail "on"

off :: AutofillDetail
off = AutofillDetail "off"

name :: AutofillDetail
name = AutofillDetail "name"

[...]

and also exporting the constructor so that people can add their own ones if necessary, eg if the list of values expands. Alternatively I think just using String would be justifiable here too.

megamaddu commented 4 years ago

String seems fine to me since the rest of the dom library just uses primitives (or zero typing, in the case of css). There could be an alternative dom library which types all the fields and styles more strictly, or more often a much smaller subset like a component library.

megamaddu commented 4 years ago

It's probably a bigger issue that we currently have the autoComplete prop on every element 😅

tesujimath commented 4 years ago

It's probably a bigger issue that we currently have the autoComplete prop on every element

@spicydonuts There's an easy solution to that, in codegen/react-html-attributes.json simply move autoComplete from * to all the places it should appear, one of them being input. I don't have the complete list to hand, or I would do a quick PR.

megamaddu commented 4 years ago

Looks like it's input, textarea, select, and form. I'll add it to the PR above.