Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bkretz/feature/celcius temp #275

Merged
merged 13 commits into from
Mar 7, 2023
Merged

Bkretz/feature/celcius temp #275

merged 13 commits into from
Mar 7, 2023

Conversation

kretzbryan
Copy link
Collaborator

My initial attempt at this was to add new columns to the patient metadata(let me know if this is the correct terminology for adding a new field object to the metadata).
Example:
{ name: 'temperature', type: 'decimal', label: 'Temperature', shortLabel: 'Temp.', unit: '°F', range: { min: 80, max: 150 }, conversion: 'celsius', }, { name: 'celsius', type: 'decimal', unit: '°C', range: { min: 26.5, max: 65.5 }, conversion: 'temperature', }

This didn't work because it no longer matched the patient model on the backend, to the app would crash.

You can see in the up-to-date branch that I added a conversion object to the temperature column of the patient metadata. That was my attempt to solve the issue of the app crashing, but since there was no specific column for the celsius field, it was never added to the Ringdown payload. I did add the celsius object statically to the Ringdown class file, which is most definitely not ideal. With the way the temperature column is currently, I could map through the patient metadata and return two FieldMetadata classes if there is a conversion field within the column (possibly in the ModelMetadata class), but I'm trying to get an understanding of how and when the ModelMetaData is being used on the backend.

@fwextensions
Copy link
Collaborator

The ModelMetadata instances are just thin wrappers that make it easy to convert the arrays of field specifications to forms needed by the server or client. That way, the field names, types, ranges, etc. are only defined once.

Adding a field to the metadata or even the server model doesn't automatically add it to the database, though. To do that, a migration script has to be added to server/migrations/, which can then add a column for the new field.

For the F/C fields, it may make sense to create a control like the one I made for blood pressure, so we don't need to force it into the generic field structure. I think it's probably simplest to keep using F for the temperature field in the database.

We could also look at using a composite field, as described in #273, to combine the temp value with a selected units.

@kretzbryan
Copy link
Collaborator Author

kretzbryan commented Mar 1, 2023

If i create a control for F/C fields, are we expecting to add a new field for celsius to the metadata? My biggest issue with this was the fact that the app crashed on login when I did this, and the error I got was a db related error (shown below).

Screenshot 2023-02-28 at 8 05 25 PM

Screenshot 2023-02-28 at 8 01 21 PM

@fwextensions
Copy link
Collaborator

are we expecting to add a new field for celsius to the metadata?

I don't see any reason to. The F temp can remain the source of truth, and can be used to generate the values for both F/C fields.

@kretzbryan
Copy link
Collaborator Author

kretzbryan commented Mar 2, 2023

are we expecting to add a new field for celsius to the metadata?

I don't see any reason to. The F temp can remain the source of truth, and can be used to generate the values for both F/C fields.

Ah okay so just handle the conversion logic in a control for temp and using local state to store the celsius value as opposed to handling it in the Ringdown class?

@kretzbryan
Copy link
Collaborator Author

A couple more things. The stylesheet you created for bpfield works well with the temp field. Should I name the stylesheet/subsequent classes something a little more generic and use it for the temperature control as well? Also, since the input handling Fahrenheit is going to throw an error either way, is it safe to say we don't need validation state for Celsius?

@fwextensions
Copy link
Collaborator

fwextensions commented Mar 4, 2023

Maybe the CSS and basic structure could be rolled up into a FormMultiField component or something, that would render the label and whatever child elements are passed to it. Then the BP and temp controls could both use it.

It may make sense to include validation on the Celsius field, if only because it would be slightly weird seeing an error on the F field if you're typing it in C. But then is it weird seeing the error on both fields? Maybe start without it and we can see how it feels.

@kretzbryan
Copy link
Collaborator Author

Maybe the CSS and basic structure could be rolled up into a FormMultiField component or something, that would render the label and whatever child elements are passed to it. Then the BP and temp controls could both use it.

Good idea. Let me know what you think of the Component wrapper I made for this.

It may make sense to include validation on the Celsius field, if only because it would be slightly weird seeing an error on the F field if you're typing it in C. But then is it weird seeing the error on both fields? Maybe start without it and we can see how it feels.

Sweet. I'll hold off on handling celsius error handling for the time being.

@fwextensions
Copy link
Collaborator

Looks good! Can you move the FormMultiField files into /Components, so they're with the other form components?

@fwextensions fwextensions merged commit 9eabe17 into main Mar 7, 2023
@fwextensions fwextensions linked an issue Mar 7, 2023 that may be closed by this pull request
@fwextensions fwextensions deleted the bkretz/feature/celcius-temp branch March 7, 2023 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EMS: User needs ability/option to enter temperature in Celsius
2 participants