-
Notifications
You must be signed in to change notification settings - Fork 38
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
[MDS-5049] TSF view and edit by roles #2830
Conversation
services/core-web/src/components/mine/Tailings/MineTailingsTable.tsx
Outdated
Show resolved
Hide resolved
services/core-web/src/components/mine/Tailings/MineTailingsTable.tsx
Outdated
Show resolved
Hide resolved
I ended up not moving the tailing files into the common package for this ticket |
services/minespace-web/common/components/tailings/AssociatedDams.tsx
Outdated
Show resolved
Hide resolved
services/core-web/common/components/tailings/AssociatedDams.tsx
Outdated
Show resolved
Hide resolved
services/minespace-web/common/components/tailings/AssociatedDams.tsx
Outdated
Show resolved
Hide resolved
services/minespace-web/src/components/dashboard/mine/tailings/TailingsTable.js
Outdated
Show resolved
Hide resolved
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.
Looks great! Just added a couple suggestions on importing Icons for better performance. I'm guessing moving these components into the common package just became too much of a can of worms?
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! yah the moving of the components kind of complicated things, so I ended up just focusing on the story ac for this one. |
…/bcgov/mds into mds-5049-tsf-view-and-edit-roles
icon: <EyeOutlined className="icon-sm padding-sm--right" />, | ||
clickFunction: (_event, record) => { | ||
handleNavigateToEdit(event, record, (userAction === "edit" ? "editView" : "view")); | ||
}, |
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.
What does editView mean? Seems like it would be simpler to just make it "edit".
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.
Ah editView was setup because we have a scenario where if a user can edit tsf and dams, they will be able to see the edit and view options in the action menu. If the user with edit access clicks on the dam view button, it will bring them to a dam page with all the user inputs disabled. When the user moves back out of the dam page and into the TailingsSummaryPage we still want them to have the ability to edit things. Having the editView helps let us know the user should still be able to edit. Let me know if it doesn't make sense we can talk about it further.
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.
Looks like we are already tracking their permissions on the page that consumes it, so I'm not sure that we need this other variable to also indicate that they should have permission to edit. That userAction still really looks to me like it wants to be a boolean- it is always compared to a string literal. A string constant would be better than a string literal, but a boolean I really think would be best. You're either in view mode or edit mode, and the page itself can track whether the user has permission to change 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.
The boolean change has now been applied.
const [canEditTSF, setCanEditTSF] = useState(false); | ||
const { renderConfig, components, routes, isCore } = useContext(TailingsContext); | ||
const { Loading } = components; | ||
const action = userAction ? userAction : "edit"; |
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.
set defaults when destructuring props like userAction="edit" on line 86.
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 has been adjusted now.
f2e105a
Objective
MDS-5049
has core_edit_tsf
ormds_minespace_proponents
roles