-
Notifications
You must be signed in to change notification settings - Fork 27
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
Signal count by classification pie chart #2816
base: main
Are you sure you want to change the base?
Conversation
…by classification
src/Frontend/Components/AccordionWithPieChart/__tests__/AccordionWithPieChart.test.tsx
Outdated
Show resolved
Hide resolved
[PieChartCriticalityNames.HighCriticality]: OpossumColors.orange, | ||
[PieChartCriticalityNames.MediumCriticality]: OpossumColors.mediumOrange, | ||
[PieChartCriticalityNames.NoCriticality]: OpossumColors.darkBlue, |
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 wonder whether it would make sense to have this mapping from Criticality to color once in like shared-styles.ts and then reuse it both for the icons and for the pie chart. Conceptually these are the same thing.
Then you could also get rid of the enum PieChartCriticalityNames and convert the strings to text in test.ts
@@ -24,16 +24,6 @@ export enum ButtonText { | |||
Delete = 'Delete', | |||
} | |||
|
|||
export enum ProjectStatisticsPopupTitle { |
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 like that you got rid of this enum :)
Maybe we can make the PieChartCriticalityNames also obsolete (see other comment)
src/Frontend/Components/ProjectStatisticsPopup/ProjectStatisticsPopup.util.ts
Show resolved
Hide resolved
@@ -141,6 +158,17 @@ function getLicenseDataFromVariants( | |||
|
|||
const variantCriticality = attributions[attributionId].criticality; | |||
|
|||
const variantClassification = attributions[attributionId].classification; |
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.
Consistency check to see whether I understand these Pie charts:
Generally we aggregate all unresolved attributions and group them by some normalized license name ("stripped license name"). Then for each pie chart, we group them further by some value:
- most frequent licenses: no further grouping, just direct counts of how often a "stripped license" appears
- criticality: each "stripped license" gets assigned the highest criticality found on any of its corresponding attributions
- classification: same as criticality, goes by max classification
- incomplete attributions: works directly on attributions (not licenses)
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.
That's also my understanding here
Summary of changes
Added a pie chart showing signal count by classification in the project statistics popup.
Context and reason for change
Users should be able to get a quick overview of the classifications present in their signals.
How can the changes be tested
Open a .opossum file containing classifications and check the project statistics popup
closes #2802