-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat(frontend, backend): connect rxdb #6
Conversation
ae666ad
to
fbd61db
Compare
@jcha-686 |
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.
Self-review:
I'm relatively happy with the code as it stands. I don't like how boilerplatey the RxDB stuff is, and while I've tried to add abstraction, I made sure not to spend too much time on it. I'd be keen for any ideas on simple ways to improve it.
Ready to open PR for review now.
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.
As far as I can tell, it's looking good - just that one point (not sure if it was covered before)
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.
As far as I can tell LGTM. Nice job with the enum serialize/deserialize btw.
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! Just some feedback on functionality, will continue looking through the code itself:
I was able to follow the testing steps and everything persisted to the database:
Testing config:
Browser: Chrome
Device: Windows Laptop
Editing the patient's name or another input field in a browser didn't update in the other browser automatically without me refreshing. The name fields update in the tab name, but not the input itself.
Some changes I'd like to see are:
-
When you initially open the Patient Details page, the page is blank minus the table at the bottom. I know this is kind of OOS for your PR, but we can't really show that in the demo, so it'd be good if you changed this.
-> Could you remove the table when no patient is selected, and have a message like "No patient selected yet!". Also, if you already have patients, could the user be automatically redirected to the first tab? -
Since all fields are nullable in the DB, could you add asterisks next to the required fields on the FE side - just so the user knows what fields should be filled in?
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.
The code looks nice and clean to me, just 2 nitpicks:
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.
LGTM, functionality is as described and code has no issues as far as I can see.
I noticed some general UI/usability issues that will need to be addressed in future, but they're well out of scope here so I put them in triage on the scrum board.
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.
🚀 LGTM
Description
Closes this ticket.
lol:
This is a LARGE PR with a lot of potential to break things. It also includes a number of design changes. I'm very open to suggestions and requests for improvement.
Testing:
npx nx serve backend
npx nx serve
I would recommend checking that changes are persisted to the database by using the GraphQL playground.
You can check concurrency by opening a second browser, browser profile, or a new browser window in a private browsing mode (multiple tabs do NOT show concurrency and aren't explicitly supported in the current config).
Everything should update concurrently
except for ENUM fields, which will require you to unload the patient and reload it (page refresh, or select a different patient).after debounce delay.You can simulate going "offline" by shutting off the server, making some changes, then turning it back on.
You can simulate deleting a patient by editing the
deletedAt
field of a patient in the GraphQL playground.Changes:
dateOfBirth
field returning null unexpectedly by adding custom scalar typeTo do before merging
Please let me know if any of these are out of scope or should be in a separate PR.
Add testsdeletedAt
worksTo do in other PRs
lastUpdatedAt
winsminUpdatedAt
in thepatientReplicationFeed
queryType of change
How Has This Been Tested?
TODO.
Basic local manual testing.
Test Configuration:
Checklist: