-
Notifications
You must be signed in to change notification settings - Fork 43
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
🐛 inherit review data from multiple archetypes #1493
Conversation
249e925
to
e8573fb
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1493 +/- ##
=======================================
Coverage 40.22% 40.22%
=======================================
Files 143 143
Lines 4539 4539
Branches 1096 1096
=======================================
Hits 1826 1826
Misses 2616 2616
Partials 97 97
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
ac929b1
to
171da19
Compare
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 look like 90% good to go -- I just have a few suggestions to help reduce some complexity and repetition.
...t/src/app/pages/applications/applications-table-assessment/applications-table-assessment.tsx
Outdated
Show resolved
Hide resolved
isFetching: isFetchingArchetypes, | ||
error: archetypesFetchError, | ||
refetch: fetchArchetypes, | ||
} = useFetchArchetypes(); |
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.
Just a thought -- when the application assessment + analysis tables are combine, it would be good to rework the data fetching & data cross-references into its own hook. That way the hook can do something like...
heavyApplication = {
...applicaiton,
archetypes: mapRefsToObject(applicaiton.archetypes, archetypes),
isReviewed: ....,
hasReviewedArchetype: ...,
}
...then the component code can just wire things together
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.
+1
client/src/app/pages/applications/components/application-detail-drawer/review-fields.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/components/application-detail-drawer/review-fields.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.
A rogue console.log
is still in there, and 1 more suggestion
item: ReviewDrawerLabelItem, | ||
labelText?: string | number | ||
) => { | ||
console.log("generateReviewLabel", item, labelText); |
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.
Don't forget to remove the console log.
client/src/app/pages/applications/components/application-detail-drawer/review-fields.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/components/application-detail-drawer/review-fields.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: ibolton336 <ibolton@redhat.com>
Signed-off-by: ibolton336 <ibolton@redhat.com>
Signed-off-by: ibolton336 <ibolton@redhat.com>
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.
LGTM
client/src/app/pages/applications/components/application-detail-drawer/review-label.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: ibolton336 <ibolton@redhat.com>
https://issues.redhat.com/browse/MTA-1382
https://issues.redhat.com/browse/MTA-1537