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.
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) O3-3018: Adding metric tiles to the refApp homepage. #1075
base: main
Are you sure you want to change the base?
(feat) O3-3018: Adding metric tiles to the refApp homepage. #1075
Changes from 15 commits
b5e71d5
3a3f706
41b5fbe
5e30eec
c8fec9a
c0df520
5f5c033
a498561
80e7ab7
70b1138
d54bca9
91ca09b
f8a23de
85c7d8f
a52fb6d
afd8ef8
31b1421
f151e66
530e4cf
2dccd12
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there a use case for this? Is this tied to other data within the home screen that has a search functionality or is this purely a way to chain params to the url?
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.
Majorly chaining params together, although I am not sure I fully understand your question, but it is tied to
sessionLocation
from the home page.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 tag seems redundant
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.
Can we just move the className to the header and eliminate the need for the extra
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 becomes tricky when we're adding tiles from other esms. Can we maintain this in the esm home?
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.
My though process behind this:
openmrs-esm-home
since that slot should be totally generic and agnostic of whatever is loaded into it.esm-active-visits-app
,esm-appointments-app
) but need to live inside a single container for the purposes of uniformity of styling/ alignment, and also so that they are loaded into the extension slot withinopenmrs-esm-home
as a single unit instead of three individual tiles.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 is my understanding;
MetricSlot
) introduces the Metrics slot which is specific to the home.What am suggesting is this specific component defining the metrics-slot should be
esm-home
andpatient-management
let alone theesm-visits-app
. It's not a tied to the visits esm and implementers who decide not to include theesm-visits
in the importmap would have this. But implementations that haveesm-home
should be able to take advantage of themetrics-tiles-slot
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.
Aah, I see what you mean! Which begs the question, where within
patient-management
would you suggest I define this? Could there be a way to represent this logic independent of any of the modules? I hope my understanding of what you said was correct.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 slot would move to the
esm-home
and the different tiles would be attached in the routes.json of their respective esmsThere 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.
Same comments apply to this component about cleaning this up
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.
Was this a placeholder?
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.
Are you referring to:
[anythingElse: string]: any;
?If so then this was just a contingency.
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 think it's best not to have this. That forces us to confront any short-comings in the types instead of ignoring them... and if someone really needs to use a different property, there's always
(visit as any).prop = value
as a work-around.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.
Thank you for the clarity, will resolve this.
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.
Let's delete this