code-for-charlottesville / housinghub

GNU General Public License v3.0
14 stars 23 forks source link

Map property input fields to backend #40

Closed M-Fineout closed 4 years ago

M-Fineout commented 4 years ago

These changes should bring the property fields in line with the property schema with the exception of IDs and the vouchers not accepted array.

@thinkharderdev Do we need an array for both vouchers accepted and vouchers not accepted? This seems redundant.

thinkharderdev commented 4 years ago

These changes should bring the property fields in line with the property schema with the exception of IDs and the vouchers not accepted array.

@thinkharderdev Do we need an array for both vouchers accepted and vouchers not accepted? This seems redundant.

I think so. The way I think about it is that vouchers accepted is the set of vouchers known to be accepted and the vouchers not accepted is the set of vouchers known to not be accepted. It is possible that there will be vouchers where it is not known whether they are accepted or not.

M-Fineout commented 4 years ago

I will add a vouchers not accepted array at initialization. As far as changing the pattern of how we're handling forms, I think we need @dgoldstein1 to weigh in as he instilled the pattern we are using currently. I am open to other methods myself. For now I will fix conflicts and implement the new fields.

dgoldstein1 commented 4 years ago

@thinkharderdev

I really think we need to use some sort of form library (like https://jaredpalmer.com/formik/ maybe). I think some of the stuff we are doing is an anti-pattern, like dispatching a redux action on every form field change event. We should definitely chat through this at the next meeting. Can we get a list going of things we should solidify in the frontend / backend?

From my experience, this is not a design anti-pattern. In fact, there are several use cases I can think of off the top of my head (e.g. user navigating to home and back and losing add property data) where localizing state to something like formik would hurt us. For our use case, I can't see any performance hits. React is smart in only rendering data that changes as long as our reducers are written well, and a lot of these libraries do what we're doing under the hood.

I don't usually have opinions about front-end frame works / libraries, but I don't think we need to use an external library for this functionality. Instead we should:

thinkharderdev commented 4 years ago

@thinkharderdev

I really think we need to use some sort of form library (like https://jaredpalmer.com/formik/ maybe). I think some of the stuff we are doing is an anti-pattern, like dispatching a redux action on every form field change event. We should definitely chat through this at the next meeting. Can we get a list going of things we should solidify in the frontend / backend?

From my experience, this is not a design anti-pattern. In fact, there are several use cases I can think of off the top of my head (e.g. user navigating to home and back and losing add property data) where localizing state to something like formik would hurt us. For our use case, I can't see any performance hits. React is smart in only rendering data that changes as long as our reducers are written well, and a lot of these libraries do what we're doing under the hood.

I don't usually have opinions about front-end frame works / libraries, but I don't think we need to use an external library for this functionality. Instead we should:

  • have good errors on the back-end instead of deferring that logic to validating forms on the front end. This keeps the front end "dumb" so that all it does is display text and send it to the back end
  • keep the front end and back end APIs clean and connected so that the forms look identical to the backend API

I've not done React dev for a while but I was just going off of https://github.com/redux-form/redux-form (which ultimately references https://blog.isquaredsoftware.com/2017/01/practical-redux-part-7-forms-editing-reducers/) which is pretty explicit about doing what we're doing here being an anti-pattern. At the very least triggering a re-render on every keypress seems like overkill.

M-Fineout commented 4 years ago

@thinkharderdev and @dgoldstein1

I plan to push any new changes regarding this issue from a fresher branch.

Thanks!