openmrs-esm-core
openmrs-esm-core copied to clipboard
(feat) O3-3178: Add a reusable dashboard header to the styleguide
Requirements
- [x] This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as
feat,fix, orchore, among others). See existing PR titles for inspiration.
For changes to apps
- [ ] My work conforms to the O3 Styleguide and design documentation.
If applicable
- [ ] My work includes tests or is validated by existing tests.
- [ ] I have updated the esm-framework mock to reflect any API changes I have made.
Summary
The page-header is re-built multiple times across pages. It should be a re-usable UI / code component as explained in detail by Ciarán Duffy here.
With this PR, a reusable component is created and tested on the openmrs-esm-patient-management using yalc and import map overrides.
Sibling pull request in openmrs-esm-patient-management with implementation
https://github.com/openmrs/openmrs-esm-patient-management/pull/1236
Implementaion Screancast in openmrs-esm-patient-management:
Screenshots
Patient lists
Appointments
service queues
Related Issue
Other
Hi @brandones, @denniskigen , @cduffy2 I request for a review here. Thanks!
Hi @Twiineenock
There should be a different pictogram on each page. This should be made configurable. You can find all of the pictograms on this page of the design documentation
For pages that only have one view, like appointments, there's no need to show a view switcher.
It looks like the Home page still has the word home written twice on it. Also on the other pages, the clinic name is specified as 'Community Outreach', is that intentional?
Overall, I still see a few things that need to be refined on this before it should be merged. Thanks for your efforts on it and let me know if my comments are clear enough. If not, feel free to come to the Design Office hours again and we can work through it again together
@ciaranduffy, that feed-back was helpful to me. Implementation is not yet made to the home and laboratory since these exist in separate repositories. If all is good for the those headers in the openmrs-esm-patient-management, I will proceed to the home and laboratory.
For the clinic name, I am using the one selected from these after login:
You surely can let me know if this is not the case for clinic names
Thanks!
Furthermore @ciaranduffy, following these pictograms, except the labs and appointments pictograms, the home, service queues and patient lists pictograms I must use are not explicitly clear to me!
@Twiineenock the clinic name is not the same as the location name.
Clinic name might be something like 'Ampath Clinic'. Within Ampath Clinic there will be many different locations like 'Inpatient Ward', 'Outpatient ward' etc. The clinic name should not change across pages or locations.
As for the pictograms, I believe Paul already sent you the correct pictograms to use for each page.
@Twiineenock For RefApp, I would suggest we use a generic clinic name like WellnessPoint Demo Clinic and implement it in a way that that is configurable in one place for implementations.
@Twiineenock the clinic name is not the same as the location name.
Clinic name might be something like 'Ampath Clinic'. Within Ampath Clinic there will be many different locations like 'Inpatient Ward', 'Outpatient ward' etc. The clinic name should not change across pages or locations.
As for the pictograms, I believe Paul already sent you the correct pictograms to use for each page.
@ciaranduffy, thank you for the feedback. Does this mean we are removing the location name from the interface? Is there any other UI component, aside from the PageHeader, that currently displays the location name? I am concerned that if we remove the location name from the header, our clients may no longer have access to this information. Could you please clarify?
@brandones thanks! for the feedback, I am cleaning it to meet that view!
Hi @Twiineenock, is there a user story for the location name to be displayed on the screen at all times? If not, I think where it currently displays in the location switcher in the user preferences section is sufficient.
Thanks
the clinic name is not the same as the location name.
Clinic name might be something like 'Ampath Clinic'. Within Ampath Clinic there will be many different locations like 'Inpatient Ward', 'Outpatient ward' etc. The clinic name should not change across pages or locations.
@ciaranduffy I don't think we have any construct called "Clinic name." It sounds like a higher-level location, like a location that contains other locations. I assume we wouldn't want to just show the same string everywhere that uses the implementation? Especially for online implementations serving many geographically disparate areas. But I think the only options we have right now are to show the location there, or to show a string that is set in the configuration and that always displays exactly the same for all users.
CC @gracepotma @ibacher @dkayiwa @bmamlin if any of you have thoughts on this
If there's an ongoing Talk thread or something I'm not aware of, please link it.
Greetings @ciaranduffy, @brandones @denniskigen!
I've refactored the component(s) to introduce a reusable PageHeaderContainer and a dedicated PageHeader component.. Furthermore, I've registered the page-header pictograms in the styleguide and utilized them within this pull request:.
One key area I'd love your feedback on is:
Am I using the the most suitable approach for handling clinic name configuration ?
Thanks @brandones. I think having a single configurable string of text would be the best solution here. It should capture the name of the place (or authority) where all possible locations a user can choose are located. So whether that's a single site like 'Wellness Clinic' or aname that captures multiple sites like 'North Western Health Board', that's ok.
It definitely shouldn't be the user's location though. Because this will create a lot of confusion in the service queues page, and appointments page, which we will be working on iterating and improving quite soon. The reason it would be confusing is that the user's location could be 'Triage' but they could be viewing the list of appointments for 'Consultation Room #2'.
So for now, best to have a single configurable piece of text for the secondary text and the page name for the primary text.
@ciaranduffy is it optional? In the sense that some one may choose not to have it displayed?
@Daniel Kayiwa @.***> No, I think this would be a required part of the setup / configuration process. It powers a core part of the UX which helps the user understand the context of the page they're viewing in Clinic Management screens.
On Wed, 24 Jul 2024 at 10:58, dkayiwa @.***> wrote:
@ciaranduffy https://github.com/ciaranduffy is it optional? In the sense that some one may choose not to have it displayed?
— Reply to this email directly, view it on GitHub https://github.com/openmrs/openmrs-esm-core/pull/1084#issuecomment-2247283686, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZYMZTCBFNJMTNZL37X5UJ3ZN5UCPAVCNFSM6AAAAABLCIBHHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBXGI4DGNRYGY . You are receiving this because you were mentioned.Message ID: @.***>
-- Ciarán Duffy Designer & Board Member +49 152 28458104 sonderdesign.org http://www.sonderdesign.org || duff.work http://www.duff.work He / Him / His
I would suggest for our RefApp example clinic: 'Wellness Point Demo Clinic'
On Wed, 24 Jul 2024 at 11:29, Ciarán Duffy @.***> wrote:
@Daniel Kayiwa @.***> No, I think this would be a required part of the setup / configuration process. It powers a core part of the UX which helps the user understand the context of the page they're viewing in Clinic Management screens.
On Wed, 24 Jul 2024 at 10:58, dkayiwa @.***> wrote:
@ciaranduffy https://github.com/ciaranduffy is it optional? In the sense that some one may choose not to have it displayed?
— Reply to this email directly, view it on GitHub https://github.com/openmrs/openmrs-esm-core/pull/1084#issuecomment-2247283686, or unsubscribe https://github.com/notifications/unsubscribe-auth/AZYMZTCBFNJMTNZL37X5UJ3ZN5UCPAVCNFSM6AAAAABLCIBHHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBXGI4DGNRYGY . You are receiving this because you were mentioned.Message ID: @.***>
-- Ciarán Duffy Designer & Board Member +49 152 28458104 sonderdesign.org http://www.sonderdesign.org || duff.work http://www.duff.work He / Him / His
-- Ciarán Duffy Designer & Board Member +49 152 28458104 sonderdesign.org http://www.sonderdesign.org || duff.work http://www.duff.work He / Him / His
Hi @brandones , @ciaranduffy !
I have updated to code to meet the above requirements! Furthermore update the openmrs-esm-patient-management PR
Thanks!
Looks great, thank you @Twiineenock . I've requested a second opinion on the API before we merge this, since it will be much harder to change after we merge.
And thanks @brandones for recommending the linking of the framework in first place. With yalc, this kind of development is awesome!
I will be glad to fix the API if requirements change!
A note based on @ibacher 's ideas regarding clinicName/implementationName, here for posterity: eventually we want to allow implementers to have this come from the location hierarchy. For orgs such as AMPATH, where one implementation serves like 90 clinics, having just one string for the whole implementation doesn't really make sense. So we'll want to allow implementers to set a location tag that denotes a clinic, and then we will display the nearest ancestor location to the current location that has the clinic tag.
For other sites, for which one implementation more or less serves one clinic, they can continue just using the implementationName config option.
A note based on @ibacher 's ideas regarding
clinicName/implementationName, here for posterity: eventually we want to allow implementers to have this come from the location hierarchy. For orgs such as AMPATH, where one implementation serves like 90 clinics, having just one string for the whole implementation doesn't really make sense. So we'll want to allow implementers to set a location tag that denotes a clinic, and then we will display the nearest ancestor location to the current location that has the clinic tag.For other sites, for which one implementation more or less serves one clinic, they can continue just using the
implementationNameconfig option.
Are we to implement code logic out of this in this PR?
"So we'll want to allow implementers to set a location tag that denotes a clinic, and then we will display the nearest ancestor location to the current location that has the clinic tag."
@Twiineenock No, in this PR we're just doing the config element, as you've done, but we'll call it implementationName.
Working on those fixes!
So the clients can do this
<PageHeader title="Patient Lists" illustration={<PatientListsPictogram />} className={} />
or this
<PageHeader className={}>
<PageHeader title="atient Lists" illustration={<PatientListsPictogram />} className={}/>
<P>children</P>
</PageHeader>
But can't do this:
<PageHeader title="atient Lists" illustration={<PatientListsPictogram />} className={} children={}/>
or
<PageHeader title="atient Lists" illustration={<PatientListsPictogram />}>
<P>children</P>
</PageHeader>
This is probably what you meant, but to fix your comment above:
Clients should be able to do this:
<PageHeader title="Patient Lists" illustration={<PatientListsPictogram />} />
or this
<PageHeader>
<PageHeaderContent title="Patient Lists" illustration={<PatientListsPictogram />} />
<p>children</p>
</PageHeader>
But not this:
<PageHeader title="Patient Lists" illustration={<PatientListsPictogram />}>
<PatientHeaderContent title="Something else" illustration={<AnotherPictogram />}
I have written docstrings for the components which include usage examples.