(feat) O3-2749 Service Queues - Add Patient Banner to "Add patient to queue" form #953
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +115 B (0%) Total Size: 2.81 MB ℹ️ View Unchanged
|
mseaton
reviewed
Jan 26, 2024
There was a problem hiding this comment.
Before reviewing in more detail, it might be worthwhile to separate out the changes due to the pre-commit hooks from those related to this ticket. Can we get the pre-commit hooks to execute on a checkout without changes, and review/commit that, and then this PR can build on that and show just what is meaningful?
@denniskigen / @ibacher - what's up with these changes that come in from the git hooks? What is your typical approach to dealing with these?
@chibongho - note, there is also a failing check below
It's mostly to ensure that new translation strings are correctly added to all files and unused translation strings are removed. They're done in Git hooks mostly so it's automatic for people, though this may be the result of people by-passing those checks.
In general, I just mark the translation files as "Viewed" in the PR list and focus on the code. |
|
While this isn't per se a reason to stop this PR, I think we're at the point where we should refactor the patient header into a standard React component. Creating a number of slots all with the same name is not how the extension system is intended to be used... |
|
Yeah, I bypassed the pre-commit hook to add the translations as well in my recent commit because it was adding a lot of noise... also, although it's great to clean these up automatically, I'm a little fearful that it may be a bit aggressive in deleting translations... if we remove a message code, potentially temporarily, we have to re-translate when we add it back in, but I guess that's the a trade off... |
Tend to agree, but this does allow an implementation to add in their own custom header in a way that just having this be a component would not, correct @ibacher ? |
mogoodrich
approved these changes
Jan 26, 2024
Yeah, I suppose that's true. |
donaldkibet
approved these changes
Jan 27, 2024
|
Ok, thanks for the reviews. I'll go ahead and separate the translation stuff into a separate PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
Make it so that the form to add a paitent to a queue has a
<PatientBanner />so we can see who we are adding. This is consistent with the UI in the appointments app when adding new appointments.Note: a lot of translation files are changed from the git pre-push hook. I only actually changed patient-search.component.tsx and appointments-header.component.tsx
cc: @ojwanganto
Screenshots
Related Issue
Other