-
Notifications
You must be signed in to change notification settings - Fork 0
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
Inventory #4
Conversation
…ter to view all inventory items
@@ -63,6 +63,11 @@ export const schema = [ | |||
plural: 'medications', | |||
relations: { patient: { belongsTo: 'patient' } }, | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is necessary given that we don't know what pouchdb.ts will look like with the new migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting it prevented the search function from working in InventoryRepository. I'm guessing it's because the file imports relationalDb from puchdb.ts which requires the use of the schema. I'm not sure what other way to do it since InventoryRepository requires some sort of database either way. The rest of the repositories seem to do it this way so I figure it's safe to do so as well.
rank: '', | ||
type: '', | ||
crossReference: '', | ||
reorderPoint: ('' as unknown) as number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This casting feels strange to me. Why not start with a number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the value is set through a text input, it's a string. However, strings and numbers don't appear to overlap enough to simply cast from one to the other. Turns out you have to make it unknown first. I would try to change the input to a numerical one but I didn't see any templates on the inputs folder so I decided to leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @gcrt0701 ! I'm quite impressed -- this is a hefty PR that uses a ton of cool techniques I intend to use in my future React development.
I would also appreciate if you could resolve the merge conflicts with src/shared/components/navbar/pageMap.tsx
. If you would like help with that, let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, @gcrt0701! Great work on all of this!
Fixes HospitalRun#2486.
Changes proposed in this pull request:
Note: pull requests without proper descriptions may simply be closed without further discussion. We appreciate your contributions, but need to know what you are offering in clearly described format. Provide tests for all code that you add/modify. If you add/modify any components update the storybook. Thanks! (you can delete this text)