Skip to content
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-4026 Add an icon for vitals and biometrics on the left nav #2079

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Munyua123
Copy link

@Munyua123 Munyua123 commented Oct 28, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

Screenshots

image

image

Related Issue

Other

@Munyua123
Copy link
Author

@denniskigen I have made the changes on the icon using the Activity Icon.

Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes, @Munyua123. It's close. I've left a few more recommendations.

Comment on lines 6 to 7
import { ConfigurableLink } from '@openmrs/esm-framework';
import { ActivityIcon } from '@openmrs/esm-framework';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we consolidate these imports?

@@ -4,6 +4,8 @@ import last from 'lodash-es/last';
import { useTranslation } from 'react-i18next';
import { useLocation } from 'react-router-dom';
import { ConfigurableLink } from '@openmrs/esm-framework';
import { ActivityIcon } from '@openmrs/esm-framework';
import styling from './dashboardextension.scss';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import styling from './dashboardextension.scss';
import styles from './dashboardextension.scss';

Can we rename this file to dashboard-extension.scss for consistency? https://o3-docs.openmrs.org/docs/coding-conventions#naming

@@ -22,13 +24,18 @@ export const DashboardExtension = ({
const location = useLocation();
const navLink = useMemo(() => decodeURIComponent(last(location.pathname.split('/'))), [location.pathname]);

const renderIcon = title === 'Vitals & Biometrics' ? <ActivityIcon className={styling.icons} /> : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const renderIcon = title === 'Vitals & Biometrics' ? <ActivityIcon className={styling.icons} /> : null;
const renderIcon = title === 'Vitals & Biometrics' ? <ActivityIcon className={styles.icons} /> : null;

return (
<div key={path}>
<ConfigurableLink
className={classNames('cds--side-nav__link', { 'active-left-nav-link': path === navLink })}
to={`${basePath}/${encodeURIComponent(path)}`}
>
{t(title)}
<span className={styling.menu}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<span className={styling.menu}>
<span className={styling.menu}>

Ditto

align-items: center;
}

.icons {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would .icon be more preferable here? It's just one icon, right?

@Munyua123 Munyua123 changed the title O3-4026 Add an icon for vitals and biometrics on the left nav (feat) O3-4026 Add an icon for vitals and biometrics on the left nav Oct 29, 2024
@Munyua123
Copy link
Author

@denniskigen I have made the changes that you had suggested. I have also made a change to use the switch case method, so that someone who is implementing more icons can just add the icons easily

@Munyua123
Copy link
Author

@denniskigen I have made the changes and added the icons for all of them but the immunizations tab. It seems that it has not been added to the openmrs framework and also there is none in the carbon react one. Also the icon i have used for the patient summary has not been added yet to the framework so i have used the one in the carbon react icons.

@Munyua123
Copy link
Author

Munyua123 commented Oct 29, 2024

I have added the screenshot also
image

Comment on lines 39 to 68
const menuIcon = (title: string) => {
switch (title) {
case 'Patient Summary':
return <Report className={styles.icon} />;
case 'Vitals & Biometrics':
return <ActivityIcon className={styles.icon} />;
case 'Orders':
return <ShoppingCartIcon className={styles.icon} />;
case 'Medications':
return <MedicationIcon className={styles.icon} />;
case 'Results':
return <ChartAverageIcon className={styles.icon} />;
case 'Visits':
return <CalendarHeatMapIcon className={styles.icon} />;
case 'Allergies':
return <WarningIcon className={styles.icon} />;
case 'Conditions':
return <ListCheckedIcon className={styles.icon} />;
case 'Attachments':
return <DocumentAttachmentIcon className={styles.icon} />;
case 'Programs':
return <TableIcon className={styles.icon} />;
case 'Appointments':
return <EventScheduleIcon className={styles.icon} />;
default:
return null;
}
};

const renderIcon = menuIcon(title);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an object lookup like the following is more preferable. It has less code and better type inference, and it should be straightforward to pop in more icons:

Suggested change
const menuIcon = (title: string) => {
switch (title) {
case 'Patient Summary':
return <Report className={styles.icon} />;
case 'Vitals & Biometrics':
return <ActivityIcon className={styles.icon} />;
case 'Orders':
return <ShoppingCartIcon className={styles.icon} />;
case 'Medications':
return <MedicationIcon className={styles.icon} />;
case 'Results':
return <ChartAverageIcon className={styles.icon} />;
case 'Visits':
return <CalendarHeatMapIcon className={styles.icon} />;
case 'Allergies':
return <WarningIcon className={styles.icon} />;
case 'Conditions':
return <ListCheckedIcon className={styles.icon} />;
case 'Attachments':
return <DocumentAttachmentIcon className={styles.icon} />;
case 'Programs':
return <TableIcon className={styles.icon} />;
case 'Appointments':
return <EventScheduleIcon className={styles.icon} />;
default:
return null;
}
};
const renderIcon = menuIcon(title);
type MenuTitle = keyof typeof MenuIcons;
const MenuIcons = {
Allergies: WarningIcon,
Appointments: EventScheduleIcon,
Attachments: DocumentAttachmentIcon,
Conditions: ListCheckedIcon,
Medications: MedicationIcon,
Orders: ShoppingCartIcon,
'Patient Summary': Report,
Programs: TableIcon,
Results: ChartAverageIcon,
Visits: CalendarHeatMapIcon,
'Vitals & Biometrics': ActivityIcon,
} as const;
const menuIcon = (title: MenuTitle) => {
const Icon = MenuIcons[title];
return Icon ? <Icon className={styles.icon} /> : null;
};
const renderIcon = menuIcon(title as MenuTitle);

@Munyua123
Copy link
Author

@denniskigen The above is noted and will be implemented. I had a question will it be okay if i add the missing icons to the framework? or do i have to wait for directive on this

@denniskigen
Copy link
Member

Yes, please go ahead and add them in Core.

@Munyua123
Copy link
Author

@denniskigen I have added all the icons , kindly review

@denniskigen
Copy link
Member

@paulsonder @ciaranduffy could you give this a design review?

@paulsonder
Copy link

@denniskigen Works for me. We'll just need to coordinate on making sure these are reflected in the design docs.

@@ -22,13 +37,40 @@ export const DashboardExtension = ({
const location = useLocation();
const navLink = useMemo(() => decodeURIComponent(last(location.pathname.split('/'))), [location.pathname]);

type MenuTitle = keyof typeof MenuIcons;

const MenuIcons = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Munyua123 for this, what will be the approach for adding icons for packages that do not live in the patient chart ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CynthiaKamau The approach may be adding the icons in their respective packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants