Skip to content
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.

Sign up for GitHub

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

feat(ui): aggregate risk scenarios in risk matrix to avoid bloating #1407

Merged
merged 10 commits into from
Jan 24, 2025
Merged

feat(ui): aggregate risk scenarios in risk matrix to avoid bloating #1407

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
90f3f88
feat: new cell component for risk matrices
Axxiar Jan 21, 2025
48c1c03
fix(tests): handle undefined displayedData case
Axxiar Jan 21, 2025
070ca19
feat: risk scenarios displayed in tooltip + bubbles
Axxiar Jan 22, 2025
b563fe9
fix(ui): use fixed scale for bubbles size
Axxiar Jan 23, 2025
c385993
fix(ui): scrollable tooltip and minor ui fix
Axxiar Jan 23, 2025
51eaa41
Merge branch 'main' into CA-819-aggregate-risk-scenarios-in-risk-matr…
Axxiar Jan 23, 2025
ff91fb4
fix(test): fixed test failing
Axxiar Jan 24, 2025
82e3c76
fix: base case for cells w/ 34+ risk scenarios
Axxiar Jan 24, 2025
78ee9f5
feat: code support for possibility to toggle bubbles
Axxiar Jan 24, 2025
dcbbf36
fix: better cells popup id, arrow under tooltip
Axxiar Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions frontend/src/lib/components/RiskMatrix/Cell.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
<script lang="ts">
import { isDark } from '$lib/utils/helpers';
import { popup, type PopupSettings } from '@skeletonlabs/skeleton';

export let cell;
export let cellData: Array<any> = [];
export let dataItemComponent;
Comment on lines +5 to +7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add proper TypeScript types for props.

The current prop declarations lack proper typing, which reduces type safety. Consider adding proper TypeScript interfaces and type validation.

+interface CellLevel {
+    hexcolor: string;
+}
+
+interface Cell {
+    level: CellLevel;
+}
+
+interface RiskScenario {
+    ref_id: string;
+    // Add other required properties
+}
+
+interface SvelteComponent {
+    // Add required component properties
+}
+
-export let cell;
-export let cellData: Array<any> = [];
-export let dataItemComponent;
+export let cell: Cell;
+export let cellData: RiskScenario[] = [];
+export let dataItemComponent: typeof SvelteComponent;

Committable suggestion skipped: line range outside the PR's diff.

export let useBubbles = true;

const bubbleMinCount = 1;
const maxBubbleSize = 4.5;
const bubbleSizeRanges = [
{ max: 3, value: 1.5 },
{ max: 5, value: 2 },
{ max: 8, value: 2.5 },
{ max: 13, value: 3 },
{ max: 21, value: 3.5 },
{ max: 34, value: 4 }
];

Comment on lines +11 to +21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Revisit bubble size thresholds.
Your code dynamically grows bubble sizes but might conflict with user feedback requesting a uniform/fixed scale. Also, bubbleMinCount is set to 1, but your PR notes a threshold of 3. Consider aligning this logic with the agreed bubble sizing policy.

-const bubbleMinCount = 1;
+const bubbleMinCount = 3;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const bubbleMinCount = 1;
const maxBubbleSize = 4.5;
const bubbleSizeRanges = [
{ max: 3, value: 1.5 },
{ max: 5, value: 2 },
{ max: 8, value: 2.5 },
{ max: 13, value: 3 },
{ max: 21, value: 3.5 },
{ max: 34, value: 4 }
];
const bubbleMinCount = 3;
const maxBubbleSize = 4.5;
const bubbleSizeRanges = [
{ max: 3, value: 1.5 },
{ max: 5, value: 2 },
{ max: 8, value: 2.5 },
{ max: 13, value: 3 },
{ max: 21, value: 3.5 },
{ max: 34, value: 4 }
];

let popupClick: PopupSettings;
if (cellData.length) {
popupClick = {
event: 'click',
target: 'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-'),
placement: 'top'
};
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve popup target generation and add validation.

The current implementation has potential issues:

  • Long concatenated strings of ref_ids could exceed length limits
  • No validation of ref_ids existence
 if (cellData.length) {
+    const validRefIds = cellData
+        .filter(item => item.ref_id)
+        .map(item => item.ref_id);
+    if (validRefIds.length !== cellData.length) {
+        console.warn('Some items are missing ref_id');
+    }
     popupClick = {
         event: 'click',
-        target: 'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-'),
+        target: `popup-${validRefIds.slice(0, 3).join('-')}`,
         placement: 'top'
     };
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (cellData.length) {
popupClick = {
event: 'click',
target: 'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-'),
placement: 'top'
};
}
if (cellData.length) {
const validRefIds = cellData
.filter(item => item.ref_id)
.map(item => item.ref_id);
if (validRefIds.length !== cellData.length) {
console.warn('Some items are missing ref_id');
}
popupClick = {
event: 'click',
target: `popup-${validRefIds.slice(0, 3).join('-')}`,
placement: 'top'
};
}


$: classesBubbleSize = (itemCount: number) => {
for (const range of bubbleSizeRanges) {
if (itemCount <= range.max) {
return `width: ${range.value}rem; height: ${range.value}rem`;
}
}
return `width: ${maxBubbleSize}rem; height: ${maxBubbleSize}rem`;
};
Comment on lines +31 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify bubble sizing logic for fixed scale.

Since we're moving to a fixed bubble size as per requirements, this reactive statement can be simplified.

-$: classesBubbleSize = (itemCount: number) => {
-    for (const range of bubbleSizeRanges) {
-        if (itemCount <= range.max) {
-            return `width: ${range.value}rem; height: ${range.value}rem`;
-        }
-    }
-    return `width: ${maxBubbleSize}rem; height: ${maxBubbleSize}rem`;
-};
+$: classesBubbleSize = () => `width: ${bubbleSize}rem; height: ${bubbleSize}rem`;

Committable suggestion skipped: line range outside the PR's diff.

$: classesCellText = (backgroundHexColor: string) => {
return isDark(backgroundHexColor) ? 'text-white' : '';
};
</script>

{#if useBubbles && cellData.length >= bubbleMinCount && dataItemComponent}
<div
class="flex flex-wrap items-center space-x-1 justify-center h-full cursor-pointer whitespace-normal overflow-y-scroll hide-scrollbar group {classesCellText(
cell.level.hexcolor
)}"
style="background-color: {cell.level.hexcolor};"
data-testid="cell"
use:popup={popupClick}
>
<div
class="bg-surface-900/70 rounded-full flex justify-center items-center text-center text-white transition-colors group-hover:bg-surface-900/100 duration-300"
style="{classesBubbleSize(cellData.length)};"
>
{cellData.length}
</div>
<div
class="card bg-surface-300 text-black p-4 z-20 max-h-56 overflow-y-scroll"
data-popup={'popup' + 'data' + '-' + cellData.map((e) => e.ref_id).join('-')}
>
{#each cellData as item}
<svelte:component this={dataItemComponent} data={item} />
{/each}
<!-- not displayed because of overflow-y-scroll on parent -->
<div class="arrow bg-surface-300" />
</div>
</div>
{:else}
<div
class="flex flex-wrap flex-col items-center justify-center h-full [&>*]:pointer-events-none whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText(
cell.level.hexcolor
)}"
style="background-color: {cell.level.hexcolor};"
data-testid="cell"
>
{#if dataItemComponent}
{#each cellData as item}
<svelte:component this={dataItemComponent} data={item} />
{/each}
{:else}
<div class="mx-auto text-center">{cellData}</div>
{/if}
</div>
{/if}
24 changes: 7 additions & 17 deletions frontend/src/lib/components/RiskMatrix/RiskMatrix.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<script lang="ts">
import { buildRiskMatrix } from './utils';
import Cell from './Cell.svelte';

import * as m from '../../../paraglide/messages';
import type { ComponentType } from 'svelte';
Expand All @@ -13,6 +14,7 @@
const grid = parsedRiskMatrix.grid;
const risk = parsedRiskMatrix.risk;
export let showRisks = false;
export let useBubbles = false;

const displayedRiskMatrix = buildRiskMatrix(grid, risk);
export let data: Array<any> | undefined = undefined;
Expand Down Expand Up @@ -79,23 +81,11 @@
{/if}
</div>
{#each row as cell, j}
<div
class="flex flex-wrap items-center space-x-1 justify-center h-full [&>*]:pointer-events-none whitespace-normal overflow-y-scroll hide-scrollbar {classesCellText(
cell.level.hexcolor
)}"
style="background-color: {cell.level.hexcolor};"
data-testid="cell"
>
{#if displayedData}
{#if dataItemComponent}
{#each displayedData[i][j] as item}
<svelte:component this={dataItemComponent} data={item} />
{/each}
{:else}
<div class="mx-auto text-center">{displayedData[i][j]}</div>
{/if}
{/if}
</div>
{#if displayedData}
<Cell {cell} cellData={displayedData[i][j]} {dataItemComponent} {useBubbles} />
{:else}
<Cell {cell} {dataItemComponent} />
{/if}
{/each}
{/each}
<div />
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/lib/components/RiskMatrix/RiskScenarioItem.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

<p class="whitespace-nowrap">
{#if data.strength_of_knowledge && data.strength_of_knowledge.symbol !== undefined}
<sup class="font-mono text-lg">{data.strength_of_knowledge.symbol}</sup>
<span class="font-mono text-lg">{data.strength_of_knowledge.symbol}</span>
{/if}
<span>{data.ref_id}</span>
</p>
3 changes: 3 additions & 0 deletions frontend/src/routes/(app)/(internal)/risk-assessments/[id=uuid]/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

export let data;
const showRisks = true;
const useBubbles = true;
const risk_assessment = data.risk_assessment;

const modalStore: ModalStore = getModalStore();
Expand Down Expand Up @@ -267,6 +268,7 @@
data={currentCluster}
dataItemComponent={RiskScenarioItem}
{showRisks}
{useBubbles}
/>
</div>
<div class="flex-1">
Expand All @@ -277,6 +279,7 @@
data={residualCluster}
dataItemComponent={RiskScenarioItem}
{showRisks}
{useBubbles}
/>
</div>
</div>
Expand Down
Loading