Rendering and markup for statistic tab #3

ViktoriiaNakoryk · 2024-02-15T14:17:17Z

created statistics tab and implemented it's rendering

MarcMcIntosh

Looks good so far :)

I've left some comments about the ui and small bits of refactoring that can be done.

Once those are done, the messages for the event bus will need to be added. But I would keep the stub data, but move into a storybook story so it can be run without needing a server running.

MarcMcIntosh · 2024-02-15T15:53:10Z

src/components/Statistic/Statistic.tsx

+  const [isLoaded, setIsLoaded] = useState<boolean>(false);
+  const [refactTable, setRefactTable] = useState<RefactTableData | null>(null);
+  const { host, tabbed } = useConfig();
+  const { backFromChat } = useEventBusForChat();


pass this as a prop, i've not made the event bus a context so (for now) this hook will recreate the state

i fixed it, but because of moving backFromChat to the parent component there was a problem in test, so I mocked there useEventBusForChat

Eeyup i saw, I've left a few comments, one that fixes the test and the other one which is a general question about if statistic should have it's own event bus?

MarcMcIntosh · 2024-02-15T15:54:36Z

src/components/Statistic/Statistic.tsx

+import { useEventBusForChat } from "../../hooks";
+import { useConfig } from "../../contexts/config-context";
+
+const table: { data: string } = {


this data stub could be added to storybook, it'll give you a visual feedback loop with out needing a backend

src/__fixtures__ contains some data stubs, could table.data be moved there?

src/components/Chart/Chart.tsx

src/components/Table/Table.tsx

src/components/Table/Table.module.css

src/lib/render/RenderStatistic.tsx

src/services/refact.ts

MarcMcIntosh · 2024-02-15T17:32:05Z

package.json

@@ -71,6 +71,8 @@
    "@vitest/coverage-v8": "^1.1.0",
    "@vitest/ui": "^1.1.0",
    "classnames": "^2.3.2",
+    "echarts": "^5.4.3",


Interesting :)... I've not seen this library before, I'll need to check the bundle size but I'd imagine it's tree shakable

run npx vite-bundle-visualizer if you want check the bundle size 👍🏻

src/components/Sidebar/Sidebar.tsx

… in browser

MarcMcIntosh

A couple of changes todo, and a question about the number format :)

MarcMcIntosh · 2024-02-19T17:21:17Z

src/components/Statistic/Statistic.tsx

+import { useEventBusForChat } from "../../hooks";
+import { useConfig } from "../../contexts/config-context";
+
+const table: { data: string } = {


src/__fixtures__ contains some data stubs, could table.data be moved there?

MarcMcIntosh · 2024-02-19T17:45:24Z

src/components/Chart/Chart.tsx

+import React from "react";
+import { Box, Text } from "@radix-ui/themes";
+import { RefactTableImpactDateObj } from "../../services/refact";
+import ReactEcharts from "echarts-for-react";


Reading the useage guide https://github.com/hustcc/echarts-for-react?tab=readme-ov-file#usage

it looks like there's a way to manually to reduce bundle size, i think that would be a good idea to try because when I run npx vite-bundle-visualizer echarts is about 2mb and it seems not all of echarts is being used.

MarcMcIntosh · 2024-02-19T17:55:33Z

src/components/Sidebar/Sidebar.tsx

+      {isOpenedStatistic ? (
+        <Statistic
+          onCloseStatistic={handleCloseStatistic}
+          backFromChat={backFromChat}


ahh, i see why backFromChat this is added here, it's because it also has to be rendered in the side bar.

🤔 that's a tricky one.... maybe statistic needs it's own event bus hook 🤔

is there a PR for adding this to vscode?

yeap, I also think that may be it is better to create own event bus for statistic. I am going to do it today.

I've connected rendered statistic with vscode, but pr for vscode isn't created yet because I don't have permission yet

src/components/Sidebar/Sidebar.tsx

MarcMcIntosh · 2024-02-19T17:59:50Z

src/components/Table/Table.tsx

+    } else {
+      const convertedNumber = Number(
+        cellValue
+          .toLocaleString("en-US", {


what's the intended format / conversion that's happening here?

numbers in table are like this: 563892, 64563 so they have a lot of digits, and to make that numbers readable I used toLocaleString to format the number so it has commas in the correct position(564731=> 56,4731) and after that rounded them to 2 digits after comma and and also added k, so the final result is 56.47k

good point lots of digits would clutter the ui,
what would happen with a number with multiple commas in it? are we likely to see a number that big?

Small bug here, after 100000 the result is NaNk.

it was found it when I was trying to test/guess the result for different inputs, most of them fail so I might be misunderstanding something.

Here’s the tests i ran, add them somewhere in the project like src/Table/formatCellValue.test.tsx and run it with npm test -- src/Table/formatCellValue.test.tsx to recreate the issue :)

// src/Table/formatCellValue.test.tsx import { describe, test, expect } from "vitest"; function formatCellValue(cellValue: number | string): string { const convertedNumber = Number(cellValue.toLocaleString("en-US", {style: "decimal",maximumFractionDigits: 0,minimumFractionDigits: 0,useGrouping: true,}).replace(",", ".")); if (convertedNumber === 0) { return "0"; } else if (Number.isInteger(convertedNumber)) { return convertedNumber + "k"; } else { return `${convertedNumber}k`; } } describe("formatCellValue", () => { test.each<[number | string, string]>([ ["0", "0"], ["10", "10"], ["100", "100"], ["1000", "1k"], ["10000", "10k"], ["564731", "56,47k"], ["100000", "100k"], ["1000000", "1000k"], [0, "0"], [10, "10"], [100, "100"], [1000, "1k"], [10000, "10k"], [564731, "56,47k"], [100000, "100k"], [1000000, "1000k"], ])("returns %s for %s", (cellValue, expected) => { expect(formatCellValue(cellValue)).toEqual(expected); }); });

MarcMcIntosh · 2024-02-19T18:20:46Z

src/features/HistorySideBar.test.tsx

@@ -5,6 +5,12 @@ import { EVENT_NAMES_TO_CHAT } from "../events";

 import { ChatHistoryItem } from "../hooks/useChatHistory";

+vi.mock("../hooks/useEventBusForChat", () => ({


I don't think useEventBusrForChat needs to be mocked, it seems that the last call to the spy is from request caps.

It's a small update to the existing test, mock uuid and change the expected last call to postMessageSpy

it("start new chat", async () => { vi.mock("uuid", () => ({ v4: () => "foo" })); const { user, ...app } = render(<HistorySideBar />); const postMessageSpy = vi.spyOn(window, "postMessage"); const startNewChatButton = app.getByText("Start a new chat"); await user.click(startNewChatButton); expect(postMessageSpy).toHaveBeenCalledWith({type: EVENT_NAMES_TO_CHAT.NEW_CHAT }, "*"); expect(postMessageSpy).toHaveBeenLastCalledWith({ type: EVENT_NAMES_FROM_CHAT.REQUEST_CAPS, payload: { id: "foo" } }, "*") });

…ts only with that components which are used here. fixed storybooks for chart and table because of error during build

MarcMcIntosh

A few small changes and a small bug to fix 👍🏻

MarcMcIntosh · 2024-02-21T17:48:52Z

src/__fixtures__/table.ts

For readability, can you save the parsed json and use JSON.stringify when it's being exported?

const json = {...} export const TABLE = { data: JSON.stringify(json) }

src/components/Chart/Chart.tsx

MarcMcIntosh · 2024-02-21T18:11:34Z

src/components/Table/Table.tsx

+    } else {
+      const convertedNumber = Number(
+        cellValue
+          .toLocaleString("en-US", {


good point lots of digits would clutter the ui,
what would happen with a number with multiple commas in it? are we likely to see a number that big?

MarcMcIntosh · 2024-02-21T18:12:57Z

src/hooks/useEventBusForStatistic.ts

MarcMcIntosh · 2024-02-21T18:21:02Z

src/lib/render/RenderStatistic.tsx

+    return (
+      <ConfigProvider config={config}>
+        <Theme>
+          <Statistic backFromStatistic={backFromStatistic} />


looks like we're heading in the right direction :)

I've been putting large components that use/call the event bus in in the src/features directory.

Then adding renderer for it in lib so it can be exported like a component :)

MarcMcIntosh · 2024-02-21T18:51:32Z

src/components/Table/Table.tsx

+    } else {
+      const convertedNumber = Number(
+        cellValue
+          .toLocaleString("en-US", {


Small bug here, after 100000 the result is NaNk.

it was found it when I was trying to test/guess the result for different inputs, most of them fail so I might be misunderstanding something.

Here’s the tests i ran, add them somewhere in the project like src/Table/formatCellValue.test.tsx and run it with npm test -- src/Table/formatCellValue.test.tsx to recreate the issue :)

// src/Table/formatCellValue.test.tsx import { describe, test, expect } from "vitest"; function formatCellValue(cellValue: number | string): string { const convertedNumber = Number(cellValue.toLocaleString("en-US", {style: "decimal",maximumFractionDigits: 0,minimumFractionDigits: 0,useGrouping: true,}).replace(",", ".")); if (convertedNumber === 0) { return "0"; } else if (Number.isInteger(convertedNumber)) { return convertedNumber + "k"; } else { return `${convertedNumber}k`; } } describe("formatCellValue", () => { test.each<[number | string, string]>([ ["0", "0"], ["10", "10"], ["100", "100"], ["1000", "1k"], ["10000", "10k"], ["564731", "56,47k"], ["100000", "100k"], ["1000000", "1000k"], [0, "0"], [10, "10"], [100, "100"], [1000, "1k"], [10000, "10k"], [564731, "56,47k"], [100000, "100k"], [1000000, "1000k"], ])("returns %s for %s", (cellValue, expected) => { expect(formatCellValue(cellValue)).toEqual(expected); }); });

…, fixed func formatCellValue and moved it to separate file

MarcMcIntosh · 2024-02-22T10:28:16Z

src/features/Statistic.tsx

+      setRefactTable(JSON.parse(TABLE.data) as RefactTableData);
+      setIsLoaded(true);
+    }
+  }, []);


note: this hook will run only when the component mounts, it'll work for now since the data is always there

MarcMcIntosh

One last thing to do be merging and that would be to hide this under a feature flag until it's complete.

I think a features property to the config context should do the trick, and the if config.features.statistics is true the statistics button can be shown.

What do you think?

MarcMcIntosh

looks good to me 👍🏻

added rendering and markup for statistic tab

acce5fa

ViktoriiaNakoryk marked this pull request as draft February 15, 2024 14:18

ViktoriiaNakoryk added 2 commits February 15, 2024 15:29

updated package-lock

da567e4

fixed indents

77d334d

MarcMcIntosh requested changes Feb 15, 2024

View reviewed changes

ViktoriiaNakoryk added 7 commits February 16, 2024 14:40

add scroll to statistic component

2289736

created components for table, changed type in locale to be default as…

8456d04

… in browser

moved defining data from components to parent component

caf6893

fixed types

979f547

added storybooks for table and plots

aab39d1

moved defining useEventBus hook to a parent component

d986685

mocked useEventBusForChat in test

9cd68ac

MarcMcIntosh requested changes Feb 19, 2024

View reviewed changes

ViktoriiaNakoryk added 2 commits February 20, 2024 13:14

moved stub table data to separate files. replaced imported ReactEchar…

7ca23f1

…ts only with that components which are used here. fixed storybooks for chart and table because of error during build

created eventBus for statistic

f799b42

MarcMcIntosh requested changes Feb 21, 2024

View reviewed changes

ViktoriiaNakoryk added 2 commits February 22, 2024 00:28

saved the the parsed json for table data, moved Statistic to features…

d16a2fe

…, fixed func formatCellValue and moved it to separate file

changed name of test-file

ca14457

MarcMcIntosh reviewed Feb 22, 2024

View reviewed changes

MarcMcIntosh requested changes Feb 22, 2024

View reviewed changes

created feature-flag for statistic

f4809ef

ViktoriiaNakoryk marked this pull request as ready for review February 22, 2024 11:55

MarcMcIntosh approved these changes Feb 22, 2024

View reviewed changes

ViktoriiaNakoryk merged commit b8d42a7 into main Feb 22, 2024
2 checks passed

ViktoriiaNakoryk deleted the statistic branch February 22, 2024 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rendering and markup for statistic tab #3

Rendering and markup for statistic tab #3

ViktoriiaNakoryk commented Feb 15, 2024

MarcMcIntosh left a comment

MarcMcIntosh Feb 15, 2024

ViktoriiaNakoryk Feb 19, 2024

MarcMcIntosh Feb 19, 2024

MarcMcIntosh Feb 15, 2024

ViktoriiaNakoryk Feb 19, 2024

MarcMcIntosh Feb 19, 2024

MarcMcIntosh Feb 15, 2024

MarcMcIntosh Feb 15, 2024

MarcMcIntosh left a comment

MarcMcIntosh Feb 19, 2024

MarcMcIntosh Feb 19, 2024

MarcMcIntosh Feb 19, 2024

ViktoriiaNakoryk Feb 20, 2024

MarcMcIntosh Feb 19, 2024

ViktoriiaNakoryk Feb 20, 2024

MarcMcIntosh Feb 21, 2024

MarcMcIntosh Feb 21, 2024 •

edited

Loading

ViktoriiaNakoryk Feb 21, 2024

MarcMcIntosh Feb 19, 2024

MarcMcIntosh left a comment

MarcMcIntosh Feb 21, 2024

MarcMcIntosh Feb 21, 2024

ViktoriiaNakoryk Feb 21, 2024

MarcMcIntosh Feb 21, 2024

MarcMcIntosh Feb 21, 2024

MarcMcIntosh Feb 21, 2024

ViktoriiaNakoryk Feb 21, 2024

MarcMcIntosh Feb 21, 2024 •

edited

Loading

MarcMcIntosh Feb 22, 2024

MarcMcIntosh left a comment

MarcMcIntosh left a comment

		@@ -5,6 +5,12 @@ import { EVENT_NAMES_TO_CHAT } from "../events";

		import { ChatHistoryItem } from "../hooks/useChatHistory";

		vi.mock("../hooks/useEventBusForChat", () => ({

Rendering and markup for statistic tab #3

Rendering and markup for statistic tab #3

Conversation

ViktoriiaNakoryk commented Feb 15, 2024

MarcMcIntosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcMcIntosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcMcIntosh Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcMcIntosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcMcIntosh Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcMcIntosh left a comment

Choose a reason for hiding this comment

MarcMcIntosh left a comment

Choose a reason for hiding this comment

MarcMcIntosh Feb 21, 2024 •

edited

Loading

MarcMcIntosh Feb 21, 2024 •

edited

Loading