DO NOT MERGE: Request for comments: Plan 5a Proof of concept #3852

PavelVanecek · 2023-10-13T13:19:12Z

So this doesn't pass tests, I don't think it actually renders the Tooltip at all. All the problems are fixable, I am just wondering what do you think about the general direction before I sink the time into fixing them.

In the meantime I would like to use the Pull Request as a vehicle for comments and discussion.

Please focus on:

Overall architecture and data flow
Breaking API changes
Any changes we want to bring to master independent of the refactor to minimize the PR size and reduce risk?

Please do not yet pay attention to:

Variable naming
Component split
File names
any

Description

As described in #3717:

The Tooltip component is now purely a marker. It renders nothing. It only cleans its props, adds some defaults, and pushes everything into a React.Context
generateCategoricalChart no longer needs to search for the Tooltip Element in DOM because it has all its data in the context. It gets some precomputed data and global state and passes it down to a new component:
TooltipRenderer This is grouping all the small individual renders that used to be split in three places before:
1. Tooltip.render
2. generateCategoricalChart.renderCursor
3. generateCategoricalChart.renderTooltip

Related Issue

#3823

Motivation and Context

#3717

How Has This Been Tested?

I tested it won't work, please don't merge it yet.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
I have added a storybook story or extended an existing story to show my changes
All new and existing tests passed.
Only discuss, no merge

PavelVanecek · 2023-10-13T13:22:25Z

src/chart/generateCategoricalChart.tsx

@@ -2312,7 +2250,6 @@ export const generateCategoricalChart = ({
        Scatter: { handler: this.renderGraphicChild },
        Pie: { handler: this.renderGraphicChild },
        Funnel: { handler: this.renderGraphicChild },
-        Tooltip: { handler: this.renderCursor, once: true },


Tooltip no longer uses the cloning map!

PavelVanecek · 2023-10-13T13:23:14Z

src/component/Tooltip.tsx

+      // @ts-expect-error TODO
+      position: props.position,
+    };
+    setContext(newContext);


The public Tooltip will now only gather and cleanup the public props and push to context - nothing more.

we should scrutinize every effect we add, but I guess optimizations can come later once its working

PavelVanecek · 2023-10-13T13:24:02Z

src/context/chartContext.tsx

+  y: boolean;
+};
+
+export type ChartContextType = {


The context might grow quite a bit, we might consider splitting to one context per component.

We'll have to see how re-renders do as well

PavelVanecek · 2023-10-13T13:24:54Z

src/renderer/TooltipRenderer.tsx

Here is the actual Tooltip + Cursor rendering. Reads from props (shared global state) and context (public props).

Combining the Tooltip and the Cursor generally seems like a great refactoring. Do you see a way to get this in independent on the conceptual change?

Yeah so without the conceptual change the component of TooltipWithCursor will accept merged props from both. There might be slight complications since they are both rendered at different times (not sure why?). I will look into that.

PavelVanecek · 2023-10-13T13:26:03Z

The more I work on the codebase the less I am convinced we can have a clean split between 5a ("Push") and 5b ("Pull"). It might end up as a combination of both, either way.

This could be a special case because generateCategoricalChart never reads any props from Tooltip - so it doesn't need its context. Or maybe I haven't found those places yet.

ckifer

I like the direction! We'll have to be careful about how unwieldy that context gets, it could quite literally get as bad as generateCategoricalChart itself lol

src/chart/generateCategoricalChart.tsx

ckifer · 2023-10-16T15:56:20Z

src/component/Tooltip.tsx

+      // @ts-expect-error TODO
+      position: props.position,
+    };
+    setContext(newContext);


we should scrutinize every effect we add, but I guess optimizations can come later once its working

ckifer · 2023-10-16T15:57:27Z

src/context/chartContext.tsx

+  y: boolean;
+};
+
+export type ChartContextType = {


We'll have to see how re-renders do as well

ckifer · 2023-10-16T15:58:57Z

src/renderer/TooltipRenderer.tsx

+import { isNumber } from '../util/DataUtils';
+import { ContentType } from '../component/Tooltip';
+
+type Props = {


going forward I think we should be specific about how we name prop types, instead of renaming on export (even though in this case this is scoped to the file)

src/renderer/TooltipRenderer.tsx

ckifer · 2023-10-23T21:33:28Z

I think tests are basically passing besides snaps?

PavelVanecek · 2023-10-23T21:37:05Z

There's still couple of tests red. As I'm going through them I'm discovering small problems. I want to get all tests green to make sure I didn't miss any roadblock, and then I move on to 5b.

…

On Tue, Oct 24, 2023, 08:33 Coltin Kifer ***@***.***> wrote: I think tests are basically passing besides snaps? — Reply to this email directly, view it on GitHub <#3852 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIMTCU7LLHF2QM4PN6TKT3YA3PDHAVCNFSM6AAAAAA57EYAJWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZWGA2TOMBVGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

ckifer · 2023-10-23T21:38:12Z

Sounds good!

PavelVanecek · 2023-11-11T01:58:38Z

I can't get the latest changes to work. In the name of performance optimization, this PR introduced mutable variables outside of state: HHongSeungWoo@830bd00 and I can't figure out if it's meant to be that way and I should follow that or if that's an oversight.

HHongSeungWoo · 2023-11-11T07:09:11Z

Not using the state was an intentional change. Currently, the tooltip is being rerendered by outer components, so there is no need to trigger state change. but you can change it if necessary

PavelVanecek · 2023-11-11T07:47:57Z

I don't have enough insight to tell if it's necessary or not but I can tell it looks like an oversight. I recommend to add unit tests that show that it is intentional and what unnecessary re-renders does it prevent. Otherwise you can have someone come and go "This is not The React Way(tm)" and change it.

PavelVanecek · 2023-11-11T07:50:04Z

And a comment too 😬

PavelVanecek · 2023-11-15T11:55:56Z

Sibling PR: #3965

PavelVanecek · 2023-11-17T09:23:25Z

Declining in favour of #3965

@nikolasrieble

## Description Following up from @nikolasrieble's suggestion in #3852 (comment) I am moving towards merging Cursor and Tooltip component. Cursor depends on Tooltip props anyway so it's a natural refactor. Baby steps. First I move the Cursor rendering as-is to its own component. In a followup PR, I will make it read props from Tooltip directly and remove `renderCursor` method from generateCategoricalChart. ## Related Issue #3717 ## Motivation and Context Small step towards breaking down generateCategoricalChart ## How Has This Been Tested? `npm test`, storybooks ## Screenshots (if appropriate): ## Types of changes - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist: - [X] My code follows the code style of this project. - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [X] I have added tests to cover my changes. - [X] I have added a storybook story or extended an existing story to show my changes - [X] All new and existing tests passed.

PavelVanecek requested review from nikolasrieble and ckifer October 13, 2023 13:19

PavelVanecek added the DO NOT MERGE label Oct 13, 2023

PavelVanecek commented Oct 13, 2023

View reviewed changes

PavelVanecek changed the title ~~DO NOT MERGE, WORK IN PROGRESS: Plan 5a Proof of concept~~ DO NOT MERGE: Request for comments: Plan 5a Proof of concept Oct 13, 2023

PavelVanecek force-pushed the plan-5a-poc branch from 81680d7 to caf3235 Compare October 14, 2023 02:57

ckifer reviewed Oct 16, 2023

View reviewed changes

PavelVanecek force-pushed the plan-5a-poc branch 3 times, most recently from 2e1b2b2 to 22b0c6d Compare October 22, 2023 22:51

ckifer mentioned this pull request Oct 23, 2023

Refactoring mouse event #3891

Merged

9 tasks

PavelVanecek force-pushed the plan-5a-poc branch from 22b0c6d to 41ef4ed Compare October 30, 2023 09:42

PavelVanecek force-pushed the plan-5a-poc branch from 41ef4ed to 7b66cfb Compare November 11, 2023 01:56

PavelVanecek mentioned this pull request Nov 12, 2023

Split Tooltip into TooltipBoundingBox #3951

Merged

9 tasks

Plan 5a: Proof of concept

10b554c

PavelVanecek force-pushed the plan-5a-poc branch from 7b66cfb to 10b554c Compare November 15, 2023 10:52

PavelVanecek mentioned this pull request Nov 15, 2023

DO NOT MERGE: RFC: Plan 5b Proof of concept #3965

Closed

9 tasks

PavelVanecek closed this Nov 17, 2023

PavelVanecek mentioned this pull request Nov 25, 2023

Separate cursor component #3994

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DO NOT MERGE: Request for comments: Plan 5a Proof of concept #3852

DO NOT MERGE: Request for comments: Plan 5a Proof of concept #3852

PavelVanecek commented Oct 13, 2023 •

edited

PavelVanecek Oct 13, 2023

PavelVanecek Oct 13, 2023

ckifer Oct 16, 2023

PavelVanecek Oct 13, 2023

ckifer Oct 16, 2023

PavelVanecek Oct 13, 2023

nikolasrieble Nov 12, 2023

PavelVanecek Nov 12, 2023

PavelVanecek Dec 22, 2023

PavelVanecek commented Oct 13, 2023 •

edited

ckifer left a comment •

edited

ckifer Oct 16, 2023

ckifer Oct 16, 2023

ckifer Oct 16, 2023

ckifer commented Oct 23, 2023

PavelVanecek commented Oct 23, 2023 via email

ckifer commented Oct 23, 2023

PavelVanecek commented Nov 11, 2023

HHongSeungWoo commented Nov 11, 2023

PavelVanecek commented Nov 11, 2023

PavelVanecek commented Nov 11, 2023

PavelVanecek commented Nov 15, 2023

PavelVanecek commented Nov 17, 2023

DO NOT MERGE: Request for comments: Plan 5a Proof of concept #3852

DO NOT MERGE: Request for comments: Plan 5a Proof of concept #3852

Conversation

PavelVanecek commented Oct 13, 2023 • edited

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

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

PavelVanecek commented Oct 13, 2023 • edited

ckifer left a comment • edited

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

ckifer commented Oct 23, 2023

PavelVanecek commented Oct 23, 2023 via email

ckifer commented Oct 23, 2023

PavelVanecek commented Nov 11, 2023

HHongSeungWoo commented Nov 11, 2023

PavelVanecek commented Nov 11, 2023

PavelVanecek commented Nov 11, 2023

PavelVanecek commented Nov 15, 2023

PavelVanecek commented Nov 17, 2023

PavelVanecek commented Oct 13, 2023 •

edited

PavelVanecek commented Oct 13, 2023 •

edited

ckifer left a comment •

edited