Skip to content

BAH-4754 Command Palette for Bahmni#128

Open
lingeswaranTW wants to merge 2 commits into
masterfrom
BAH-4754
Open

BAH-4754 Command Palette for Bahmni#128
lingeswaranTW wants to merge 2 commits into
masterfrom
BAH-4754

Conversation

@lingeswaranTW

Copy link
Copy Markdown

No description provided.

@lingeswaranTW lingeswaranTW force-pushed the BAH-4754 branch 4 times, most recently from 77bd738 to 95f488a Compare May 28, 2026 13:38
@lingeswaranTW lingeswaranTW changed the title BAH-4754 BAH-4754 Command Palette for Bahmni May 28, 2026
@lingeswaranTW lingeswaranTW force-pushed the BAH-4754 branch 4 times, most recently from 3593597 to 8984d62 Compare June 5, 2026 06:27

@ravinderkabli ravinderkabli left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review for BAH-4754

Good start on the Command Palette configuration. Few review comments

"cmdPaletteNavRegistration": {
"id": "org.bahmni.commandpalette.nav.registration",
"extensionPointId": "org.bahmni.commandpalette.navItem",
"translationKey": "COMMAND_PALETTE_NAV_REGISTRATION",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: URLs use /bahmni-new/ prefix, but the established convention in this repo for React/new-frontend routes is /bahmni-v2/ (e.g., home/extension.json uses /bahmni-v2/registration/search, clinical/extension.json uses /bahmni-v2/clinical/{{patientUuid}}).

Same applies to lines 22, 73, and 80. If /bahmni-new/ is intentionally replacing /bahmni-v2/, could you note that in the PR description?

"icon": "fa-registered",
"newTab": true,
"order": 1,
"requiredPrivilege": "app:registration"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: All 7 nav extensions are missing the "type": "link" field. In home/v2/extension.json (the only other v2 extension file in the repo), every entry includes "type": "link". Could you add it to each nav item for consistency?

"order": 5,
"requiredPrivilege": "app:ot"
},
"cmdPaletteNavOpenMRS": {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: cmdPaletteNavOpenMRS and cmdPaletteNavBahmniWiki (line 58) use "label" as the sole display field with no translationKey. The other 7 extensions all use translationKey. In existing v2 extensions, label is only used as a supplemental fallback alongside translationKey, never as the sole display string.

This means these two items won't be translatable and have no corresponding entries in the locale files. Consider adding translationKey values and locale entries for both.

Comment thread openmrs/apps/command-palette/app.json Outdated
"commandPalette": {
"trigger": {
"type": "combination",
"keys": "cmd+k"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: The cmd modifier maps only to the macOS Command key. On Windows and Linux clinical workstations — the dominant platforms in Bahmni deployments — this won't trigger. Consider adding a ctrl+k fallback or defining both: "keys": ["cmd+k", "ctrl+k"].

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.

2 participants