astrosat / astrosat-ui

MIT License
0 stars 0 forks source link

68 material ui textfield #91

Closed RoryMacGregor88 closed 4 years ago

RoryMacGregor88 commented 4 years ago

This PR closes #68

There are a few little things with this one, not sure how to proceed.

First off, handling the success state doesn;'t seem as straightforward as the error state, as the error state comes as a default prop on the FormControl component and is therefore easy to manipulate, but there is no success equivalent that I can see.

Secondly, the input adornment seems fairly straightforward, and it makes sense to me to place the IconButton component inside, as it would always be there, so no need to separate it out. This presents some issues with the JSDoc prop types, but it's possibly just that I'm pushing this late in the evening and I'm just tired and can't see it.

Thirdly, no idea what to do with the InputLabel, as there are no designs for this case, and I heard some chattering coming from the designers about possibly using the Material default label style/animation.

Reignable commented 4 years ago

If we really want to add a success prop, we'll need to extend the Input props to add it, then you hand all the props to makeStyles and can use them in there to make decisions https://material-ui.com/styles/basics/#adapting-based-on-props. To be honest makeStyles should be receiving all the props anyway, but you've defaulted to passing an empty object.

I don't agree with puting the IconButton inside InputAdornment. That forces every adornment to have an icon button in it which may not be what is wanted. A pound sign at the start of a pound input doesn't need to be clickable for example. What goes into the adornment is an implementation concern not a library concern.

If we want to use the Material animation for the label then we need to know that now. Otherwise I'd use the shropshire forms as a guide https://app.zeplin.io/project/5dbc13c25fdea783b88ab405/screen/5f198ca5defb6687561368bd. There's props for the label or field to disable it, don't know which component just read the API pages. If we do switch then the designers should be made aware that every label must act the same.

Reignable commented 3 years ago

:tada: This PR is included in version 2.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: