modal-user-id #6
Conversation
takooja
commented
Feb 16, 2023
takooja
commented
| @@ -66,7 +67,8 @@ export function AppMenu (props: MenuProps) { | |||
|
|
|||
| <NavLink | |||
| className={`${APP_MENU_CLASS_NAME}-content-nav-item`} | |||
| to={USER_LIST_ROUTE} | |||
| // to={USER_LIST_ROUTE} | |||
| to={"/"+ `${workspace}` + "/users"} | |||
There was a problem hiding this comment.
This URL creation should be a function in the constants/route like this:
const getWorkspaceUserListRoute = (workspaceId: string) => `/${q(workspaceId)}/users`;
Also it seems there should be /workspaces/ prefix or something like that.
| @@ -38,7 +41,8 @@ export function AppNav (props: AppNavProps) { | |||
|
|
|||
| <NavLink | |||
| className={`${APP_NAV_CLASS_NAME}-section-item`} | |||
| to={USER_LIST_ROUTE} | |||
| //to={USER_LIST_ROUTE} | |||
| to={"/workspace/"+ `${workspace}` + "/users"} | |||
There was a problem hiding this comment.
Use constant function defined in constants/route like the getWorkspaceUserListRoute(workspaceId)
| @@ -20,7 +21,9 @@ export function SelectWorkspaceButton (props: SelectWorkspaceButtonProps) { | |||
| const children = props?.children; | |||
| const onClick = useCallback( | |||
| () => { | |||
| WorkspaceService.setCurrentWorkspace(workspace); | |||
| WorkspaceService.setCurrentWorkspace(workspace).then(()=>{ | |||
| RouteService.setRoute("/workspace/"+workspace?.id+"/about") | |||
There was a problem hiding this comment.
Use function defined in the constants/route for these links
| import "./UserView.scss"; | ||
|
|
||
| export interface UserViewProps { | ||
| readonly t: TFunction; | ||
| readonly className ?: string; | ||
| readonly index ?: number; | ||
| readonly item: User; | ||
| readonly arr : readonly User[] ; |
There was a problem hiding this comment.
Use better names for public properties. Like list or content.
We don't have any reason to save two bytes from writing arr instead of array.
| import "./UserView.scss"; | ||
|
|
||
| export interface UserViewProps { | ||
| readonly t: TFunction; | ||
| readonly className ?: string; | ||
| readonly index ?: number; | ||
| readonly item: User; | ||
| readonly arr : readonly User[] ; | ||
| } | ||
|
|
||
| export function UserView (props: UserViewProps) { |
There was a problem hiding this comment.
This component seems to be edited incorrectly.
View components should be independent. This component was meant to be the single user view. Not reused as a table row component for other parent view.
This appears to be some kind of table row instead and part of another parent component. That is also the reason why it takes strange arguments like index and array.
Instead, this implementation should be part of the parent component like UserListView -- and if it needs to be it's own component, it should be located beside that component as it's own component. However, probably better just use it directly in parent.
|
|
||
| const LOG = LogService.createLogger('useUrlWorkspaceName'); | ||
|
|
||
| export function useUrlWorkspaceName ( |
There was a problem hiding this comment.
There should be documentation what this hook does ... and the name probably should also hint that.
| useEffect( | ||
| () => { | ||
|
|
||
| if (workspaceByUrl !== undefined && workspace?.id === undefined){ |
There was a problem hiding this comment.
This code probably has a problem that this asynchronous operation will be started multiple times
| if(user !== undefined ){ | ||
| LOG.info("Open modal with userId", user); | ||
|
|
||
| RouteService.setRoute('/workspace/'+ urlWorkspace.id +'/users/'+ user); |
There was a problem hiding this comment.
Use constant functions from constants/route for these routes
There was a problem hiding this comment.
Also not sure if the view route should even change in this case. At least it should not move to the user view but user list view. We don't even have a user view yet. Anyway this code seems to be in wrong place since this is a DarkLayout component and it should not have this type of logic at all.
frontend/src/services/UserService.ts
Outdated
| LOG.debug("User email;", email) | ||
| const workspace = WorkspaceService.getCurrentWorkspaceId() | ||
| if (email && workspace) { | ||
| const userList = await UserService.getWorkspaceUserList(workspace) |
There was a problem hiding this comment.
Current users should not use the workspace user list to fetch records... We should have a profile end point to fetch current user information.
Also this logic should probably not be in the UserService at all. ProfileService would be better.
| this._observer.triggerEvent(WorkspaceServiceEvent.CURRENT_WORKSPACE_CHANGED); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static async _initializeWorkspace () { | ||
| const list : readonly Workspace[] = await WorkspaceService.getMyWorkspaceList(); | ||
| /* const list : readonly Workspace[] = await WorkspaceService.getMyWorkspaceList(); |
There was a problem hiding this comment.
Don't leave commented code in the source code. It only makes these diffs look hard to read.
