-
Notifications
You must be signed in to change notification settings - Fork 39
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-6042] Core Permit Details Page #3190
Conversation
df187ac
to
7a174e8
Compare
services/core-web/src/components/mine/Permit/MinePermitTable.tsx
Outdated
Show resolved
Hide resolved
services/core-web/src/components/mine/Permit/MinePermitTable.tsx
Outdated
Show resolved
Hide resolved
services/core-web/src/components/mine/Permit/MinePermitTable.tsx
Outdated
Show resolved
Hide resolved
services/core-web/src/components/mine/Permit/MinePermitTable.tsx
Outdated
Show resolved
Hide resolved
services/core-web/src/components/mine/Permit/ViewPermitOverview.tsx
Outdated
Show resolved
Hide resolved
services/core-web/src/components/mine/Permit/ViewPermitOverview.tsx
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.
Nice, clean job
793b757
793b757
to
cd09488
Compare
: record.permit?.permit_amendment_guid; | ||
const menu = ( | ||
<Menu> | ||
<Menu.Item key="0"> |
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.
We should not add in the deprecated Menu.Item
syntax where it's not pre-existing.
This should get changed out for the items
syntax which is present in a few places in our codebase.
The useParams
hook should be removed- I don't think that id
is being used, but if it is it'll have to be passed down.
some unused props to remove from this component. Would be nice to also resolve that cognitive complexity issue as well but it'll take more work to refactor. |
const permitColumns = [...columns]; | ||
|
||
const partyRelationships = useSelector(getPartyRelationships); |
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 what I think happened:
- MinePermitTable was made, and used redux mapStateToProps to get partyRelationships from the state as a "prop". It was then added to propTypes
- Someone working on MinePermitInfo saw partyRelationships in the MinePermitTable propTypes and passed it as a prop
- Leading to it being both a prop and a redux state value
We could do either/or, really: take it out as a prop, or take out the useSelector redux value. I'd probably take it out as a prop because MinePermitInfo isn't otherwise using partyRelationships
Quality Gate failed for 'bcgov-sonarcloud_mds_common'Failed conditions |
Quality Gate passed for 'bcgov-sonarcloud_mds_minespace-web'Issues Measures |
Quality Gate passed for 'bcgov-sonarcloud_mds_core-api'Issues Measures |
Quality Gate failed for 'bcgov-sonarcloud_mds_core-web'Failed conditions |
Objective
ViewPermitOverview
,ViewPermitConditions
andViewPermitReports
CoreTag
component for general useMDS-6042