Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Inconsistent API for input elements #218

Open
MatthewDorner opened this issue Feb 20, 2020 · 4 comments
Open

Inconsistent API for input elements #218

MatthewDorner opened this issue Feb 20, 2020 · 4 comments

Comments

@MatthewDorner
Copy link

🐛 Bug Report

There are some form input components that have inconsistent props or missing capabilities. For instance, Checkbox has no way to set the initial value. I tried looking at the react-bootstrap docs and actually didn't see a way to set it there either, but there must be one. Because of this, Edit Patient doesn't populate the checkbox in GeneralInformation with its correct initial value. Checkbox also uses disabled instead of isEditable which is inconsistent with the other components.

Also, Typeahead doesn't have a way to disable the input. This caused the need to conditionally render either Typeahead or TextInputWithLabelFormGroup in the AppointmentDetailsForm component, which should be able to be done with a single component.

Neither Typeahead nor Checkbox currently use wrapper components in the frontend in the way TextInputWithLabelFormGroup and others do, though I'm not sure if they are needed.

Expected behavior

The various input elements should have a consistent API, at least to disable the input and populate the initial value.

@jackcmeyer
Copy link
Member

jackcmeyer commented Feb 20, 2020

@MatthewDorner This is a good find. I see a few issues that can come out of this so they can be put in their respective repos.

At first glance there should be an issue for the following:

  • Adding the ability to have a disabled Typeahead component (components repo)
  • Ability to set a default value for a checkbox (components repo)
  • Change disabled to isEditable for checkbox (components repo, breaking change)

@jackcmeyer
Copy link
Member

The reason that some fields have wrapper components is to simplify (and not duplicate) the logic for creating a form group (which allows labels to be connected to fields).

However, maybe these components are "primitive" enough (similar to just a plain textbox) that they should be moved to the components repo.

I know there is HospitalRun/components#66 which is to add validation styling, which requires form groups, I believe, which could break the wrapper components.

@MatthewDorner
Copy link
Author

The wrapper components are relevant because in components repo, everything uses disabled, while everything in the frontend uses isEditable. So by adding isEditable to those unwrapped components it makes them inconsistent with the other elements in components repo etc. We should probably just make it the same across repos.

@fox1t
Copy link
Member

fox1t commented Feb 20, 2020

Really a good call! +1 to this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants