(Feat) O3-2801 Icons across O3 Navigation MVP #2121
Conversation
Munyua123
changed the title
|
@denniskigen @gracepotma @CynthiaKamau I have made the changes. I had a problem with the last pull request with the build but this one should work just fine. KIndly review |
|
Asante SANA @Munyua123!! LGTM <3 |
| type MenuTitle = keyof typeof MenuIcons; | ||
|
|
||
| const MenuIcons = { | ||
| 'Patient Summary': ReportIcon, | ||
| 'Vitals & Biometrics': ActivityIcon, | ||
| Medications: MedicationIcon, | ||
| Orders: ShoppingCartIcon, | ||
| Results: ChartAverageIcon, | ||
| Visits: CalendarHeatMapIcon, | ||
| Allergies: WarningIcon, | ||
| Conditions: ListCheckedIcon, | ||
| Immunizations: SyringeIcon, | ||
| Attachments: DocumentAttachmentIcon, | ||
| Programs: ProgramsIcon, | ||
| Appointments: EventScheduleIcon, | ||
| } as const; | ||
|
|
||
| const menuIcon = (title: MenuTitle) => { | ||
| const Icon = MenuIcons[title]; | ||
| return Icon ? <Icon className={styles.icon} /> : null; | ||
| }; |
There was a problem hiding this comment.
There's no good reason for these to be located inside this component. They would be better located outside of it.
There was a problem hiding this comment.
Honestly, though, I think this might not be the right way to design things. This works, but only for providing icons for known menu titles, which is bad for extensibility. Better would be to update the DashboardExtensionProps to take a string of the name icon and just render it like:
document.getElementById(`omrs-icon-${icon}`) ? <Icon className={styles.icon} icon={`omrs-icon-${icon}`} /> : null;| return Icon ? <Icon className={styles.icon} /> : null; | ||
| }; | ||
|
|
||
| const renderIcon = menuIcon(title as MenuTitle); |
There was a problem hiding this comment.
No weird edge-cases and we don't have to lie to TypeScript:
| const renderIcon = menuIcon(title as MenuTitle); | |
| const renderIcon = Object.keys(MenuIcons).includes(title) ? menuIcon(title) : null; |
| @@ -0,0 +1,8 @@ | |||
| .menu { | |||
| display: flex; | |||
| align-items: center; | |||
There was a problem hiding this comment.
I don't think centering is the right option here. The items should flex from the start regardless of the width of the container.
| align-items: center; | |
| align-items: start; |
| } | ||
|
|
||
| .icon { | ||
| margin-right: 0.5rem; |
There was a problem hiding this comment.
Please use the Carbon spacing tokens for things like this (use @carbon/layout; margin: layout.$spacing-03;
Requirements
Summary
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-2801Other