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

Stack does not properly forward ref #5446

Open
iansan5653 opened this issue Dec 16, 2024 · 10 comments
Open

Stack does not properly forward ref #5446

iansan5653 opened this issue Dec 16, 2024 · 10 comments
Labels
bug Something isn't working component: Stack react

Comments

@iansan5653
Copy link
Contributor

Stack appears to accept a ref prop based on the TypeScript types, but this ref is not actually bound to the underlying element:

export function Example() {
  const ref = useRef<HTMLDivElement>(null)

  useEffect(() => console.log(ref.current))

  return <Stack {...props} ref={ref}>contents</Stack>
}

If Stack correctly forwarded its ref, the console would log an HTMLDivElement instance. However, it instead always logs null.

@Jaykhanderay
Copy link

import React, { forwardRef } from "react";

const Stack = forwardRef((props, ref) => {
return (
<div {...props} ref={ref}>
{props.children}

);
});

export default Stack;

@Rakesh31-syc
Copy link

In React, when a ref is passed to a component, that component must forward the ref to a DOM element using React.forwardRef. If the Stack component does not implement forwardRef, the ref will not be attached to the underlying DOM element, resulting in ref.current being null or undefined.

import React, { useRef, useEffect } from 'react';

export function Example(props) {
const ref = useRef(null);

useEffect(() => {
console.log(ref.current);
}, []);

return <Stack {...props} ref={ref}>contents;
}

@lesliecdubs
Copy link
Member

Primer First Responder, we'd love your help fixing this ref!

@joshblack
Copy link
Member

@iansan5653 I believe the types might be too loose for Stack which might be part of the problem here 🤔 The component does not currently support ref but the types seem to be allowing basically anything to be passed in so it has the appearance of being supported.

We've been trying to find a good way to do polymorphic as types and this seems to be a limitation with the approach we have 😅 Let me see if we can address that separately.

For Stack specifically, would it still be desirable for it to accept a ref? We can treat this as an enhancement then and get it added.

@nita939

This comment has been minimized.

@iansan5653
Copy link
Contributor Author

For Stack specifically, would it still be desirable for it to accept a ref? We can treat this as an enhancement then and get it added.

Yeah, I think this would be expected for such a low-level component. Since it's practically a lightweight div wrapper, it's definitely useful to be able to take a ref.

One simple example is that Stack could be used as a toolbar with useFocusZone if it forwarded a ref.

@Omerezoo
Copy link

export function Example() {
const ref = useRef(null)

useEffect(() => console.log(ref.current))

return <Stack {...props} ref={ref}>contents
}

@Sameer1-oo
Copy link

export function-example (){
const ref = use-Ref(null)
use-Effect (() =>console.log (ref.current)
return < stack {...props}
ref = ref {ref} = > contents
}

@romeroj748-creator
Copy link

How can the issue be resolved

Here are potential resolutions for the listed bugs in the primer/react repository, formatted within 250 characters each:

  1. useConfirm Z-Index: Ensure hostElement is removed from the DOM after use or manage z-index dynamically to prevent overlap.

  2. RelativeTime Component: Check the typings and imports in v36.27; align them with the latest documentation or update the component as needed.

  3. ActionMenu Sticky Issue: Use position: sticky instead of absolute for ActionMenu to ensure it stays within the parent container context.

  4. UnderlineNav Flex Collapse: Adjust flex properties to ensure sufficient space or modify styles to prevent item collapsing in the "more" menu.

  5. ActionList Layout Issue: Scale the ActionList.TrailingAction component to fit its parent or adjust IconButton to fit within the allocated height.

These suggestions aim to address each bug's root cause effectively!

@romeroj748-creator
Copy link

To resolve the issue of the ref not being bound to the underlying element in the Stack component, ensure that Stack uses React.forwardRef. Here's how you can implement it:

``javascript
import React, { forwardRef } from "react";

const Stack = forwardRef((props, ref) => {
return
{props.children}
;
});

export default Stack;
`

This allows the ref passed to Stack to be forwarded to the div element, enabling proper access to ref.current`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component: Stack react
Projects
None yet
Development

No branches or pull requests

10 participants