BrushY: A dummy vertical Brush for scaling/panning the YAxis #4036

kerguler · 2024-01-03T06:05:57Z

The changes clone and transform Brush as a vertical slider, but do not cast a function to it.

Description

The new BrushY keeps track of the startIndex and endIndex within a user-defined number of steps. BrushY can be linked to the YAxis to set an appropriate domain for vertical scrolling/zooming.

Related Issue

Zoom and pan - Issue #710 : #710 (comment)

Motivation and Context

Brush performs horizontal scrolling/zooming by filtering the source data as needed. BrushY removes the filtering step and provides a dummy vertical slider to scale/pan the YAxis.

How Has This Been Tested?

A demo is included in the demo/component folder to show how the link between BrushY and YAxis can be established by the user. The demo uses the useState hook to update the YAxis limits. In addition, a test is included in the test/cartesian folder (similar to the test for Brush) to confirm that the component is properly rendered.

To enable rendering the component and remove the data (domain) filtering step, changes are also done in generateCategoricalChart.tsx.

Screenshot:

Types of changes

Bug fix (non-breaking change which fixes an issue)
[ x ] 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.
[ x ] 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.

ckifer

Really appreciate you taking the time to add this, I think we'll need to think on this a little more before full release though. We want to sure we test fully to catch any edge cases and aren't adding to our already large pile of tech debt.

There are some changes in this PR that are outside of the context of the PR (version bump, eslint changes) that will need to be reverted if we are to merge this.

Will also approvals on direction from some others as this doesn't look like the direction we want to continue to move in.

Really appreciate the legwork!

.eslintrc

ckifer · 2024-01-03T06:10:52Z

demo/component/BrushYDemo.tsx

+  { day: '14', value: 10 },
+];
+
+function BrushYDemo() {


We don't really use the demo project anymore, can you add this code to the storybook instead?

Of course, as soon as I figure out how to do it :)

Should be similar code luckily! You can add this to the Brush examples: storybook - Examples - Cartesian - Brush

I'll add it as a new component as I couldn't figure out how to incorporate changes in the Brush component and have it rendered twice. I trust that the planned refactoring will enable this. Until then...

ckifer · 2024-01-03T06:11:30Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "recharts",
-  "version": "2.10.3",
+  "version": "2.10.4",


Please don't bump the version unless an official recharts release, we'll handle this internally

Of course, I'll revert this.

kerguler · 2024-01-03T06:27:30Z

Thank you @ckifer for your lightning fast response. I need this component for the react app we are developing. If you decide not to include it in the master, could you please kindly advise the best approach to achieve this functionality? At the moment, I am keeping the changes in the fork and installing them using yalc.

ckifer · 2024-01-03T06:35:49Z

We definitely want the functionality eventually! We're just trying to take a different approach with some things (as you can see the generate function is very large and hard to follow). If we can get this to a good place we can maybe merge it, but we might also wait until after some more refactoring has been done to make it easier to understand/test and so we can have a more robust API. So it just might be a little bit before this gets merged/released with that said.

I'll wait for your changes and some reviews from others and we can go from there.

In terms of publishing yourself you can publish your own fork of recharts on npm if you'd like though it's always better to follow along with master since you'll miss out. You could publish your fork and use that until we merge this if you really need this functionality now.

Thanks!

PavelVanecek · 2024-01-03T09:35:46Z

src/chart/generateCategoricalChart.tsx

+      const { offset, updateId } = this.state;
+
+      // TODO: update brush when children update
+      return cloneElement(element, {


Yes this is precisely the pattern we are trying to get rid of 😬 try to think of an API that does not require cloning, and replacing the props internally

Hm on the other hand it would make sense to have the same API as the existing Brush. After all it's the same thing, isn't it.

@PavelVanecek I couldn't find an easy answer to the problem of rendering the same component twice. Now that I am a bit more familiar with the workings of the package, it might have been easier than I thought. However, I had to make some detailed conformational changes, and played with the endIndex range. So, a separate component made sense

I believe this does not add any extra effort for future migration, if it simply stays in line with the existing brush. We will have to migrate the brush either way.

ckifer · 2024-01-03T17:46:32Z

src/cartesian/BrushY.tsx

@@ -0,0 +1,570 @@
+/**


I didn't even see this file... I wonder if we can do these changes without a new component and instead with an orientation prop or similar

The problem is not being able to render the same component twice. If that's solved, than BrushY could be integrated into Brush

Got it. We could remove the once flag and just rely on consumers to not render more than one brush in the same orientation (or throw an invariant if they do) @PavelVanecek what do you think?

The pattern to solve this is done for the Axis. A shared CartesianAxis exists, but then we also have XAxis and YAxis. Both of them internally use CartesianAxis.

nikolasrieble

Great work with user value here, please do not copy paste the component but instead extend the existing one in alignment with the libraries concepts.

i.e. we also have a CartesianAxis and then we have an XAxis component and a YAxis component.

That is a pattern that seems well applicable here. What do you think?

nikolasrieble · 2024-01-05T09:05:43Z

src/cartesian/BrushY.tsx

This file is certainly the blocking thing here.
We do not want to simply copy code with small variations.

This will reduce our velocity going forward massively.
Please do reuse the existing code for the Brush and extend it.

I agree that the existing pattern makes sense. Would it be acceptable to decommission Brush and create both an XBrush and a YBrush component?

Would be preferred, but sadly not possible without a major version increase. For now I would suggest to export Brush, and XBrush (an alias for Brush), and YBrush, and mark Brush as deprecated (but still export and not break anything).

@ckifer Would that make sense?

To be completely honest, in the next (or a future) major version I think we should remove Brush entirely and instead implement a recharts native pan and zoom feature. Brush is clunky, especially for Y values.

(That's opinion only and probably won't happen)

What's the issue with removal of the once prop and just using Brush with an orientation prop?

Also possible, but I like the consistency of patterns between components that we would have if we had YBrush and XBrush

Sure, lets get it to that state where components are shared and go from there

cesarzxk · 2024-02-27T20:07:19Z

I believe that the brush could be accepted as a child of the x and y axes, behaving in the orientation of the axis.

ckifer · 2024-02-27T21:05:12Z

Yeah I think that could make sense. Breaking API change of course. Once we get brush info into context all of this gets easier

crazy4chrissi · 2024-03-05T10:35:35Z

I find this super useful for vertical barcharts (layout="vertical"). There, I would expect the BrushY to act similar as the Brush for horizontal barcharts, i.e. control the range of dimension values. However, the default onChange does not do this because BrushY does not call this.handleBrushChange. Would be cool if this would be called by default for vertical barcharts.

kerguler added 7 commits December 28, 2023 09:09

Recharts for VEClim

1a5a649

Create BrushY.spec.tsx

ac8fc8b

Update BrushY.spec.tsx

da111b6

to-do: initial props

4aea549

fixing start/end indices

db7ee13

endIndex fix

41c04dc

minor

6b148de

ckifer requested changes Jan 3, 2024

View reviewed changes

ckifer assigned PavelVanecek, HHongSeungWoo and nikolasrieble Jan 3, 2024

PavelVanecek reviewed Jan 3, 2024

View reviewed changes

requested changes

de42859

ckifer reviewed Jan 3, 2024

View reviewed changes

nikolasrieble requested changes Jan 5, 2024

View reviewed changes

forman mentioned this pull request Apr 24, 2024

Fix range of Y axis in time-series charts xcube-dev/xcube-viewer#285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BrushY: A dummy vertical Brush for scaling/panning the YAxis #4036

BrushY: A dummy vertical Brush for scaling/panning the YAxis #4036

kerguler commented Jan 3, 2024

ckifer left a comment

ckifer Jan 3, 2024

kerguler Jan 3, 2024

ckifer Jan 3, 2024

kerguler Jan 3, 2024

ckifer Jan 3, 2024

kerguler Jan 3, 2024

kerguler commented Jan 3, 2024

ckifer commented Jan 3, 2024

PavelVanecek Jan 3, 2024

PavelVanecek Jan 3, 2024

kerguler Jan 3, 2024

nikolasrieble Jan 5, 2024

ckifer Jan 3, 2024

kerguler Jan 3, 2024

ckifer Jan 3, 2024

nikolasrieble Jan 5, 2024

nikolasrieble left a comment

nikolasrieble Jan 5, 2024

kerguler Jan 5, 2024

nikolasrieble Jan 5, 2024

ckifer Jan 5, 2024

nikolasrieble Jan 6, 2024

ckifer Jan 8, 2024

cesarzxk commented Feb 27, 2024

ckifer commented Feb 27, 2024

crazy4chrissi commented Mar 5, 2024

BrushY: A dummy vertical Brush for scaling/panning the YAxis #4036

Are you sure you want to change the base?

BrushY: A dummy vertical Brush for scaling/panning the YAxis #4036

Conversation

kerguler commented Jan 3, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshot:

Types of changes

Checklist:

ckifer 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

kerguler commented Jan 3, 2024

ckifer commented Jan 3, 2024

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

nikolasrieble 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

cesarzxk commented Feb 27, 2024

ckifer commented Feb 27, 2024

crazy4chrissi commented Mar 5, 2024