openmrs-esm-core icon indicating copy to clipboard operation
openmrs-esm-core copied to clipboard

(feat) O3-3178: Add a reusable dashboard header to the styleguide

Open Twiineenock opened this issue 1 year ago • 23 comments

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, or chore, among others). See existing PR titles for inspiration.

For changes to apps

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:

page-header.webm

Screenshots

Patient lists

patient-lists

Appointments

appointments

service queues

service-queues-header

Related Issue

Other

https://openmrs.atlassian.net/browse/O3-3178

Twiineenock avatar Jul 18 '24 09:07 Twiineenock

Hi @brandones, @denniskigen , @cduffy2 I request for a review here. Thanks!

Twiineenock avatar Jul 18 '24 10:07 Twiineenock

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 avatar Jul 18 '24 11:07 ciaranduffy

@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: clinic-name You surely can let me know if this is not the case for clinic names

Thanks!

Twiineenock avatar Jul 18 '24 15:07 Twiineenock

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 avatar Jul 18 '24 15:07 Twiineenock

@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 avatar Jul 23 '24 07:07 ciaranduffy

@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.

ciaranduffy avatar Jul 23 '24 07:07 ciaranduffy

@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?

Twiineenock avatar Jul 23 '24 09:07 Twiineenock

@brandones thanks! for the feedback, I am cleaning it to meet that view!

Twiineenock avatar Jul 23 '24 09:07 Twiineenock

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. Screenshot 2024-07-23 at 13 41 33

Thanks

ciaranduffy avatar Jul 23 '24 11:07 ciaranduffy

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.

brandones avatar Jul 23 '24 18:07 brandones

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:.

pictograms-registere

One key area I'd love your feedback on is:

Am I using the the most suitable approach for handling clinic name configuration ?

Twiineenock avatar Jul 23 '24 19:07 Twiineenock

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 avatar Jul 24 '24 08:07 ciaranduffy

@ciaranduffy is it optional? In the sense that some one may choose not to have it displayed?

dkayiwa avatar Jul 24 '24 08:07 dkayiwa

@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

cduffy2 avatar Jul 24 '24 09:07 cduffy2

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

cduffy2 avatar Jul 24 '24 09:07 cduffy2

Hi @brandones , @ciaranduffy !

I have updated to code to meet the above requirements! Furthermore update the openmrs-esm-patient-management PR

Thanks!

Twiineenock avatar Jul 24 '24 16:07 Twiineenock

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.

brandones avatar Jul 24 '24 16:07 brandones

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!

Twiineenock avatar Jul 24 '24 19:07 Twiineenock

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.

brandones avatar Jul 25 '24 14:07 brandones

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.

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 avatar Jul 25 '24 16:07 Twiineenock

@Twiineenock No, in this PR we're just doing the config element, as you've done, but we'll call it implementationName.

brandones avatar Jul 25 '24 18:07 brandones

Working on those fixes!

Twiineenock avatar Jul 26 '24 14:07 Twiineenock

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>

Twiineenock avatar Jul 28 '24 15:07 Twiineenock

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.

brandones avatar Aug 07 '24 18:08 brandones