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.

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

Working ref props for all components #2665

Open
1 task done
mdrayer opened this issue Sep 23, 2021 · 8 comments
Open
1 task done

Working ref props for all components #2665

mdrayer opened this issue Sep 23, 2021 · 8 comments
Labels
enhancement Enhancement to a current API

Comments

@mdrayer
Copy link

mdrayer commented Sep 23, 2021

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

What problem does this feature solve?

Gaining access to the component's element via ref and acting on it in some way as you would in the usual useRef() way. For example, if a dev wanted to get the dimensions of the <YAxis> component, the use of a working ref prop would make this very simple to do.

What does the proposed API look like?

The ref prop already exists on components used within charts, but they do not seem to work for all components. Possibly this is an issue with the components not properly forwarding their refs? Or possibly just an oversight with leaving the ref prop exposed?

Code example:

function MyChart() {
  const yAxisRef = useRef(null);

  useEffect(() => {
    // Access your ref and do whatever.
  }, [yAxisRef]);

  return (
    <LineChart>
      <YAxis ref={yAxisRef} />
    </LineChart>
  );
}

Currently, this gives us null for yAxisRef.current when we access it inside useEffect, whereas we would expect either the element or class for YAxis.

@ckifer ckifer added P1 High priority issues enhancement Enhancement to a current API and removed P1 High priority issues labels Dec 29, 2022
@wesoudshoorn
Copy link

Is there any update on this being implemented?

We're trying to build some custom positioning logic for the tooltip and were looking to use a ref prop on the <CartesianGrid /> component, but it does not seem to work.

@roger-tbg
Copy link

+1 here. Trying to use this on ResponsiveContainer and get a deprecation warning.

@ckifer
Copy link
Member

ckifer commented Feb 28, 2024

@roger-tbg there should only be a warning if you use ref.current.current on responsive container. That was a mistake and has been corrected backwards compatibility. The warning is just saying we'll change to the correct ref.current implementation in the next breaking release (3.0)

@roger-tbg
Copy link

@ckifer This is the relevant code:

const TopFiveWorkItemsGraph = ({
	data,
	xAxisLabel,
	yAxisLabel,
}: TopFiveWorkItemsGraphProps) => {
	const chartRef = useRef(null)
	const handleBarClick = (data: object) => {
		const rect = chartRef.current.getBoundingClientRect()
		setCurrentBarData({
			data,
			rect,
		})
	}
	const [currentBarData, setCurrentBarData] = useState({
		data: {},
		rect: {},
	})
	console.log('currentBarData', currentBarData)

	return (
		<ResponsiveContainer
			width="100%"
			height="100%"
			className={'rounded-lg bg-white'}
			ref={chartRef}
		>
			<BarChart
				width={500}
				height={300}
				data={data}
				margin={{
					top: 25,
					right: 30,
					left: 20,
					bottom: 15,
				}}
				onClick={handleBarClick}
			>
			```

@ckifer
Copy link
Member

ckifer commented Feb 29, 2024

https://github.com/recharts/recharts/blob/master/src/component/ResponsiveContainer.tsx#L75

This was done with the intent to fix the ref.current.current problem in a backwards compatible way while letting the user know that the old "wrong way" will be removed in the next major version. Looks like it logs no matter what when a ref is used which is unintentional (only should log when the incorrect ref.current.current is used).

Your usage is fine and isn't being deprecated, the log message just fires in both cases. Thanks for the heads up on that.

CC @HHongSeungWoo

ckifer pushed a commit that referenced this issue Mar 1, 2024
<!--- Provide a general summary of your changes in the Title above -->

## Description
#2665 (comment)

<!--- Describe your changes in detail -->

## Related Issue
<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an
issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps
to reproduce -->
<!--- Please link to the issue here: -->

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->

## How Has This Been Tested?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Screenshots (if appropriate):

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [x] 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:

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [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.
@ckifer
Copy link
Member

ckifer commented Mar 1, 2024

@roger-tbg warn issue should be fixed in 2.12.2

@yuki0418
Copy link

yuki0418 commented Mar 29, 2024

Hi. Is there any update on this being implemented?
I try get props from ref of , however, it seems still not working in my environment.
ref.current always return null. And there is a warning

Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

React: 17.0.2
recharts: 2.1.3

I try update versions but no working.

@ckifer
Copy link
Member

ckifer commented Mar 29, 2024

No updates - it needs worked on if anyone wants to help out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to a current API
Projects
None yet
Development

No branches or pull requests

5 participants