Add forwardRef to DrawerBody

[domschab23]

Need to pass a ref to this component to not show google places autocomplete when scrolling; rough idea in this PR https://github.com/user-interviews/rails-server/pull/24206

[kyleshike]

This feels like we're exposing the internals of the DS (which is generally best to avoid) in order to handle a pretty unique edge case in rails server. Can you elaborate a bit on the problem this is solving?

[domschab23]

This feels like we're exposing the internals of the DS (which is generally best to avoid) in order to handle a pretty unique edge case in rails server. Can you elaborate a bit on the problem this is solving?

This aims to to fix this behavior when scrolling

before.mov

with this PR https://github.com/user-interviews/rails-server/pull/24206/files you now have this

Screen.Recording.2024-03-14.at.10.34.18.AM.mov

I need to listed to a DrawerBody scrolling event since that's where the scrolling happens for a drawer per our css https://github.com/user-interviews/ui-design-system/blob/develop/src/Drawer/DrawerBody.scss#L3

Happy to fix it in some other way if there are other better ideas!

[jasonbasuil]

This feels like we're exposing the internals of the DS

@kyleshike could you elaborate a bit more on this? Is it mainly just giving the ability to access another DOM element for a very specific use case? We use forwardRef on 8 other components but would kinda agree it's a specific edge case for this container component, but generally fine if it fixes the issue Dom noted.

[kyleshike]

I guess I just feel like forwardRef gives something to this library's consumers that points to a very mutable internal of this component (the dom node itself, on which any number of actions can be taken that we may or may not want to be possible). To me, it's similar to the question of providing a className property vs supporting variants. The second of these two yields a stricter interface.

In this case, would it be possible (and maybe preferred) to add an onScroll property to this component instead?

[kyleshike]

There are some considerations that might be worthwhile here in terms of DS component design generally, primarily when we should forward a ref and when we shouldn't. But I don't think this needs to block you fixing this bug. If we decide on a heuristic here that changes this behavior, we'll just make a note to address this specific use of the drawer body

[gabescarbrough]

I'm generally of the opinion that we shouldn't be forwarding refs within DS components. Doing so is kind of creating a contract saying "you can expect this element to exist and do whatever you want with it" and that's really not a promise we want to make since individual elements within the component are implementation details that could change. In this case I think its unlikely to change since this component is pretty much a wrapper for styling but its still not a great pattern.

Given we're already doing that elsewhere its not the worst thing in the world to allow it here now but I'd prefer if we stopped forwarding refs when we hit 3.0 since its a breaking change to remove them.

In this case, would it be possible (and maybe preferred) to add an onScroll property to this component instead?

That's what I would prefer. Providing onScroll doesn't tie it directly to a specific DOM element. If the internals change later you can move the listener to the DOM element that is now relevant and not break the contract with the components using the DS component and the onScroll prop.

[domschab23]

The only annoying thing with onScrollproposal here is that I really want to add handleScroll to the to app/javascript/common/deprecated/form/google_places_autocomplete_field/google_places_autocomplete_field.jsx here
https://github.com/user-interviews/rails-server/pull/24206/files

which is a nested child component of a DrawerBody. That sort of decouples the logic for scrolling that's very specific for google_places_autocomplete_field.jsx component and would move it to where I have DrawerBodydefined. If you want to reuse google_places_autocomplete_field.jsx somewhere else then you'd lose that handleScroll logic and would always have to implement and repeat yourself when you have other DrawerBody's or any other parent scrollable elements. That doesn't seem like a good solution

[gabescarbrough]

Yeah that does sound annoying. The root issue is with with GooglePlacesAutocompleteField not displaying correctly though. Using the ref (or even onScroll) to hide the autocomplete makes the issue less apparent but really the autocomplete should stay put on its own.

Given GooglePlacesAutocompleteField is a deprecated component the long-term solution is probably to create a Google Places autocomplete field that displays correctly and we actively maintain. I get why that would be out of scope of fixing this bug, but we should generally avoid refs wherever possible. They're more of an escape hatch than a tool you want to use often.

[gabescarbrough]

After saying that I referred to the React docs for when they recommend using refs and was reminded that they are actually documented in a section called Escape Hatches.

[domschab23]

Yeah that does sound annoying. The root issue is with with GooglePlacesAutocompleteField not displaying correctly though. Using the ref (or even onScroll) to hide the autocomplete makes the issue less apparent but really the autocomplete should stay put on its own.

Given GooglePlacesAutocompleteField is a deprecated component the long-term solution is probably to create a Google Places autocomplete field that displays correctly and we actively maintain. I get why that would be out of scope of fixing this bug, but we should generally avoid refs wherever possible. They're more of an escape hatch than a tool you want to use often.

100%. We could create our own DS component where we fully control the css with google Autocomplete API to get the data

https://developers.google.com/maps/documentation/places/web-service/autocomplete