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

Jump to bottom

Proof of Concept of plan 5a #3823

Closed
1 task done
PavelVanecek opened this issue Oct 7, 2023 · 1 comment
Closed
1 task done

Proof of Concept of plan 5a #3823

PavelVanecek opened this issue Oct 7, 2023 · 1 comment
Assignees
@PavelVanecek

Comments

@PavelVanecek
Copy link
Collaborator

  • I have searched the issues of this repository and believe that this is not a duplicate.

What problem does this feature solve?

In #3717 we have discussed two approaches of refactoring recharts. This is the tracker to create a proof of concept branch where we can observe the code changes and impact on performance and API.

What does the proposed API look like?

  • Stop relying on displayName.
  • Instead of parsing children array and reading props, have each component push their existence (and their props) to a shared config.
  • generateCategoricalChart will still have to render each component but it won't have to include dozens of if layout == 'horizontal' because that information would have been pushed by the components.
  • This looks like a "Push model"
@PavelVanecek PavelVanecek mentioned this issue Oct 12, 2023
Merged
9 tasks
ckifer pushed a commit that referenced this issue Oct 12, 2023
@PavelVanecek
Adding types to renderCursor (#3850)
520e8f8 
Bonus: extracted getRadialCursorPoints function so that I can have it
typed differently from the rest: it returns different object. And added
unit test.

## Description

## Related Issue

#3717

## Motivation and Context

I think I can now make sense of the Tooltip rendering and proceed with
#3823

## How Has This Been Tested?

TS, Jest

## 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.
- [ ] I have added a storybook story or extended an existing story to
show my changes
- [X] All new and existing tests passed.
This was referenced Oct 13, 2023
Merged
Closed
ckifer pushed a commit that referenced this issue Oct 14, 2023
@PavelVanecek
Move getCursorPoints and getCursorRectangle to their own files (#3851)
1bed67d 
And add unit tests

## Description

## Related Issue

#3823

#3717

## Motivation and Context

For the PoC I want to call these two functions from outside of
`getCategoricalChart` but I figured that master will benefit from this
change too: 1. it makes the mammoth file a bit smaller and 2. makes it
easier to unit test so I did that too.

Also got rid of unnecessary lodash call in the spirit of
#3752

## How Has This Been Tested?

Jest, TS

## 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.
- [ ] I have added a storybook story or extended an existing story to
show my changes
- [X] All new and existing tests passed.
This was referenced Oct 17, 2023
Merged
Merged
ckifer pushed a commit that referenced this issue Oct 20, 2023
@PavelVanecek
Move function getUniqPayload to separate file, add tests (#3878)
fd36157 
Also this function was duplicated between Legend and Tooltip, I used the
same import in both

## Description

## Related Issue

#3823

## Motivation and Context

Deduplication and tests

## How Has This Been Tested?

Jest, TS

## 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.
- [ ] I have added tests to cover my changes.
- [ ] I have added a storybook story or extended an existing story to
show my changes
- [X] All new and existing tests passed.
@PavelVanecek
Copy link
Collaborator Author

We have decided to go forward with 5b

GMer78 pushed a commit to GMer78/recharts-1 that referenced this issue Nov 24, 2023
@PavelVanecek
Adding types to renderCursor (recharts#3850)
bb61699 
Bonus: extracted getRadialCursorPoints function so that I can have it
typed differently from the rest: it returns different object. And added
unit test.

## Description

## Related Issue

recharts#3717

## Motivation and Context

I think I can now make sense of the Tooltip rendering and proceed with
recharts#3823

## How Has This Been Tested?

TS, Jest

## 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.
- [ ] I have added a storybook story or extended an existing story to
show my changes
- [X] All new and existing tests passed.
GMer78 pushed a commit to GMer78/recharts-1 that referenced this issue Nov 24, 2023
@PavelVanecek
Move getCursorPoints and getCursorRectangle to their own files (recha…
8c694a4 
…rts#3851)

And add unit tests

## Description

## Related Issue

recharts#3823

recharts#3717

## Motivation and Context

For the PoC I want to call these two functions from outside of
`getCategoricalChart` but I figured that master will benefit from this
change too: 1. it makes the mammoth file a bit smaller and 2. makes it
easier to unit test so I did that too.

Also got rid of unnecessary lodash call in the spirit of
recharts#3752

## How Has This Been Tested?

Jest, TS

## 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.
- [ ] I have added a storybook story or extended an existing story to
show my changes
- [X] All new and existing tests passed.
GMer78 pushed a commit to GMer78/recharts-1 that referenced this issue Nov 24, 2023
@PavelVanecek
Move function getUniqPayload to separate file, add tests (recharts#3878)
2b38c4c 
Also this function was duplicated between Legend and Tooltip, I used the
same import in both

## Description

## Related Issue

recharts#3823

## Motivation and Context

Deduplication and tests

## How Has This Been Tested?

Jest, TS

## 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.
- [ ] I have added tests to cover my changes.
- [ ] I have added a storybook story or extended an existing story to
show my changes
- [X] All new and existing tests passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@PavelVanecek PavelVanecek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@PavelVanecek