-
Notifications
You must be signed in to change notification settings - Fork 77
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
OCLOMRS-1056: Unified OCL and OpenMRS Dictionary Manager Experience #750
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @suruchee , just a few suggestions, and UI/UX improvements. Also, I looked at the document with the mocks, we are missing some styles in this PR, I will attach some screenshots to show what I think we should have.
src/components/AppsMenu.tsx
Outdated
</Link> | ||
</MenuItem> | ||
<MenuItem onClick={handleClose} className={classes.menuItem}> | ||
<a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make these tags open in a new tab so that the user does not completely loose their previous page? Just adding target='_blank' rel='noreferrer'
will do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review @hadijahkyampeire, I have updated as per your suggestions. I missed a few of the UI changes by looking into the OCL site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably preferable to use the MaterialUI Link
class here.
src/components/AppsMenu.tsx
Outdated
</a> | ||
</MenuItem> | ||
<MenuItem onClick={handleClose} className={classes.menuItem}> | ||
<Link to="/" className={classes.link}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why we need to link to exactly where the user is because this is what this link does, just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to create a consistent UI feel for users between OCL and dictionary manager as you can view it in https://app.openconceptlab.org/#/ apps menu
src/components/AppsMenu.tsx
Outdated
<ClickAwayListener onClickAway={handleClose}> | ||
<MenuList autoFocusItem={open} id="menu-list-grow"> | ||
<MenuItem onClick={handleClose} className={classes.menuItem}> | ||
<a href={getOCLURL()} className={classes.link}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make this open in a new tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @suruchee , LGTM
@ibacher how do you see this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @suruchee for this! I've made some small comments on a couple of things. However, I have some slightly larger comments focused on where we're placing this and how this looks:
- Why don't we just put this in the left nav bar the way the OCL app does?
- The icons should be vertically stacked rather than horizontally spread, as this leads to a better mobile experience (and occupies less screen real estate).
src/components/AppsMenu.tsx
Outdated
</Link> | ||
</MenuItem> | ||
<MenuItem onClick={handleClose} className={classes.menuItem}> | ||
<a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably preferable to use the MaterialUI Link
class here.
src/components/OpenMRSLogo.tsx
Outdated
HTMLDivElement | ||
>> = () => { | ||
return ( | ||
<img src="/openmrs_logo_white.gif" alt={"openmrs"} style={{width:"30px"}} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to use a SVG rather than the GIF here. @suruchee: I've sent you an SVG of the OMRS logo via Slack.
src/utils/utils.ts
Outdated
@@ -119,3 +119,24 @@ export function stableSort<T>(array: T[], comparator: (a: T, b: T) => number) { | |||
}); | |||
return stabilizedThis.map(el => el[0]); | |||
} | |||
// @ts-ignore | |||
export const getEnv = forURL => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't like anything where we just use a blanket
//@ts-ignore
. - Rather than this, it's probably better to use the
OCL_SIGNUP_URL
constant we already have or even just evolve that to take the OCL application server name and we can generate the signup URL and this part from it.
@ibacher, I could not understand your comment about the UI, as the current OCL UI is |
@suruchee I was talking about this: (It's what you get if you click on the wrench while the toolbar is collapsed) |
@ibacher, do you mean to remove the appIcon and only have the wrench in the left navbar as it is in OCL or to include both? |
@suruchee I'd be in favour of removing the app icon. This is, I think, more in keeping with Material Design's overall layout that tries to keep navigation all in the left bar and reserve the spots on the app bar for in-app actions. |
PS Though on this that I hadn't expressed: we should still use that grid icon to indicate visually that it is an app switcher and not the wrench icon. |
@ibacher, please review the updated change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@suruchee Nice work! I've got a few more things, largely to ensure that we avoid as much hard-coding of both layouts and URLs as we can.
src/components/AppsMenu.tsx
Outdated
popperWithOpenNav: { | ||
display: "block", | ||
position: "absolute!important" as "absolute", | ||
top: "245px !important", | ||
left: "210px !important" | ||
}, | ||
popper: { | ||
display: "block", | ||
position: "absolute!important" as "absolute", | ||
top: "245px !important", | ||
left: "25px !important" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can do this without absolute positioning? The problem is that this becomes quite fragile, i.e., if we add anything to the NavBar we need to adjust these positionings, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we'd do something very similar to what OCL does. So here OCL defines a React Ref that's that then associated with the icon and used as the anchor for the Popper, allowing it's position to be defined relative to the button itself.
public/omrs-logo.svg
Outdated
@@ -0,0 +1,30 @@ | |||
<?xml version="1.0" encoding="UTF-8" standalone="no"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the SVG into the source tree. It can be imported directly by using
import { ReactComponent as OMRSLogo } from "./omrs-logo"
And then used as:
<OMRSLogo />
where it's needed.
src/components/AppsMenu.tsx
Outdated
icon: { | ||
marginLeft: "-10px" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used anywhere is it?
src/components/AppsMenu.tsx
Outdated
const handleClose = () => { | ||
setAnchorEl(null); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good if handleClose()
actually set the open
property of the Popper to false so that it's actually closed and not just unattached.
src/utils/constants.ts
Outdated
@@ -1,5 +1,7 @@ | |||
import { Option } from "./types"; | |||
|
|||
export const OCL_URL = "https://app.openconceptlab.org/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, my suggestion here was meant to work more like the constants below, that is:
export const OCL_URL: string =
// @ts-ignore OCL_URL is injected at runtime
window.OCL_URL ||
"https://app.staging.openconceptlab.org/#/";
Then we can adjust OCL_SIGNUP_URL
to be defined this way:
export const OCL_SIGNUP_URL = OCL_URL + "accounts/signup";
The properties defined here are injected via environment variables so that we can drive locations by configuration rather than by hard-coding like getEnv()
does.
@ibacher, I have pushed with your suggestions for position and constants but it has some issue while putting svg into source folder and using import { ReactComponent as OMRSLogo } from "./omrs-logo", I get an error which says SyntaxError: unknown: Namespace tags are not supported by default. React's JSX doesn't support namespace tags. You can set |
I see the problem... Maybe we really do need to move back to absolute positioning for the menu, just so it shows up in a sensible place... I've fixed the SVG file so that it will work appropriately. |
@ibacher, I have changed the position of the menu but it worked without absolute position but had to add overflowY: "unset" in navOpen to avoid scroll, please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Suruchi! One very small thing and I think this looks great!
src/components/NavDrawer.tsx
Outdated
@@ -114,6 +116,14 @@ export const NavDrawer: React.FC<Props> = ({ children, logout, profile }) => { | |||
} | |||
logout(); | |||
}; | |||
const handelClick = (event: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const handelClick = (event: any) => { | |
const handleClick = (event: any) => { |
src/components/NavDrawer.tsx
Outdated
setOpen(true); | ||
setAnchorEl(event.target); | ||
}; | ||
const handelClose = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const handelClose = () => { | |
const handleClose = () => { |
src/components/NavDrawer.tsx
Outdated
@@ -203,6 +213,17 @@ export const NavDrawer: React.FC<Props> = ({ children, logout, profile }) => { | |||
</ListItem> | |||
</List> | |||
<Divider component="hr" /> | |||
<List component="div"> | |||
<ListItem button dense={false} key="AppsMenu" onClick={handelClick}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ListItem button dense={false} key="AppsMenu" onClick={handelClick}> | |
<ListItem button dense={false} key="AppsMenu" onClick={handleClick}> |
src/components/NavDrawer.tsx
Outdated
<ListItem button dense={false} key="AppsMenu" onClick={handelClick}> | ||
<Tooltip title="Apps Menu"> | ||
<ListItemIcon className="list-item-icon"> | ||
<AppsMenu handleClose={handelClose} open={Boolean(anchorEl)} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<AppsMenu handleClose={handelClose} open={Boolean(anchorEl)} /> | |
<AppsMenu handleClose={handleClose} open={Boolean(anchorEl)} /> |
@@ -1,5 +1,9 @@ | |||
import { Option } from "./types"; | |||
|
|||
export const OCL_URL: string = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last bit here: can we make the default for OCL_SIGNUP_URL
be `${OCL_URL}accounts/signup/`
@ibacher, thanks for the review. I have pushed new changes, please have a review |
JIRA TICKET NAME:
Unified OCL and OpenMRS Dictionary Manager Experience
Summary:
AppIcon is added in the header and three buttons to link to OCL TermBrowser, OpenMRS Dictionary Manager, and Bulk importer.