React-window: Adding id to inner and outer tags

[smuk3c]

Is it possible to pass id as props to inner and outer tags?

[bvaughn]

As a general rule, I don't like expanding the API without a strong reason– particularly with regard to pass-thru DOM properties. I agreed to too much of this with react-virtualized (e.g. the Table component) and I think it made the library harder to maintain.

I'd like to know more about your use case. Why are the existing ref props not sufficient? Refs give you access to the _actual DOM elements_.

[nihgwu]

I think you can use a custom component with id as the tagName

const TagWithId = () => <div id='tag' />;

outerTagName = TagWithId

[bvaughn]

Hmm..I suppose that _might_ work, but the tag name props are supposed to be strings. I was thinking you could just use the ref directly. It _is_ the actual DOM element. You could even set an ID attribute on it.

[nihgwu]

Well, then I think we could change the propType to string||function, I'm using that pattern in my component, it would be much powerful without any breaking change

[bvaughn]

I don't know if that's a pattern I want to encourage. I will need to think about it more.

[nihgwu]

I agree it's not that thing for this component, but for my component(an advanced Table component which will be open-sourced soon) we need it, as I applied some styles on the row wrapper the consumer couldn't change the behavior without custom tagName, the available props to change the row's behavior is actually applied to the inner row component, and the tagName with function support already extended the use of the component.
BTW, the tagName idea is awesome 👍

[nihgwu]

In this case I guess tagName with function support would make it much easier to play with drag & drop of rows

[bvaughn]

I'd have to see how you're using it. IMO, tagName doesn't seem necessary for drag and drop. Ref should be sufficient for that also.

[nihgwu]

I haven't implemented the dnd support yet, I'm just imaging that we could use a component with Droppable from react-beautiful-dnd as the tagName, maybe I'm wrong

[smuk3c]

@bvaughn we use ID's in all of automation testing (selenium). They pointed out that's really hard for them to maintain testing without any ids present. I tried with refs but had problems because component was mounted before actual ref was created so I need additional functionality just for these purpose. It seems quite useful thing to have an option to pass props further. Why do you think that would complicate things?

[bvaughn]

I tried with refs but had problems because component was mounted before actual ref was created

I think this might suggest a misunderstanding. React sets refs synchronously after mounting— before yielding control to the browser to paint or run other JavaScript. I would not expect the delay between mount and ref setting to be observable to Selenium. Could you clarify?

They pointed out that's really hard for them to maintain testing without any ids present.

What prevents you from adding an ID attribute to the DOM element when your ref is first set?

Why do you think that would complicate things?

Any new functionality adds code and requires maintaining (documenting, testing, etc). For this reason I try to avoid adding things unless they're useful for more than edge cases. Particularly if there are alternate patterns that could work, like refs. That's why I'm trying to learn more about your case.

[smuk3c]

The thing is that I get felling that setting ids for elements much more unnatural with ref than with props. It seems like anti pattern using refs just to setup ids when I need them.

Automation team use ids for all selectors in our project. We used react-window for table with a lot of data and they felt like without ids at these places would mean complete switch for them. I didn't go further - I tried to implemented ids and make them happy.

I understand that goal is to keep api simple. Just got feeling that passing props further is quite a natural way - gives some freedom to the people using library.

[bvaughn]

After giving this more thought, the approach that seems best to me is to officially support the innerTagName / outerTagName approach mentioned above for additional props. To do this, I'm going to deprecate those two properties in favor of new properties with more appropriate names– innerElementType and outerElementType.

For example, to attach an id for testing purposes to the outer most tag, you could create a new type like this:

const outerElementType = React.forwardRef((props, ref) => (
   <div ref={ref} id="outer" {...props} />
));

Then pass it to your various list and grid components:

<FixedSizeList outerElementType={outerElementType} {...otherListProps} />,

I realize this is a bit more work than an "id" prop, for instance, but it's also a lot more flexible and _hopefully_ avoids us having this discussion again for other requests that might come up.

This will be implemented in PR #112

[fabiospampinato]

The proposed solution looks pretty ugly to me. IMHO if the className attribute is supported then the id attribute should have been supported too, I'm not sure how adding support for it would make things less maintainable. Other hypothetical attributes to support can't possibly be as important as id.

[bvaughn]

I'm not sure how adding support for it would make things less maintainable.

I'm sorry you feel that way, but (speaking from experience maintaining other libraries) I disagree. There are plenty of one-off attributes that people may want to add (e.g. aria attributes, event handlers). I have to draw the line somewhere. I choose to draw it here.

You can always wrap/decorate react-window to provide support for an alternative prop if you prefer.

[heyimalex]

@fabiospampinato Eh, className and style are necessary for styling, and ref is more idiomatic for many of the things id is generally used for, so I don't see id as very important. I understand it's necessary for some testing setups, but then _everyone_ has their little necessary props that absolutely needs to be passed through. I opened an issue here just a week ago asking for support to pass through tabIndex :)

In my opinion outerElementType is really beautiful. It gives _complete_ flexibility to users for passing things through without expanding the react-window api for every outer/innerPropName imaginable. Use hooks for memoization and you're taking a _tiny_ performance hit. You write it once, maybe with a comment pointing to this thread, and then you forget about it.

If you really want ListWithId, I thought it'd be fun to write.

import * as React from "react";
import { FixedSizeList as List } from "react-window";

function ListWithId({ outerId, outerElementType, ...props }, ref) {
  const outerElementTypeWithId = React.useMemo(() => {
    if (outerId === undefined) return outerElementType;
    return React.forwardRef((props, ref) =>
      React.createElement(outerElementType || "div", {
        ref,
        id: outerId,
        ...props
      })
    );
  }, [outerId, outerElementType]);
  return <List ref={ref} outerElementType={outerElementTypeWithId} {...props} />;
}

export default React.forwardRef(ListWithId);

[tiavina-mika]

hello, I use FixedSizedList inside a material-ui TableBody so my question is it possible to disable the default 'div' of outerElementType? because instead of TableBody then TableRow, it's TableBody, div then TableRow, and the checkbox of each row of TableBody does not show.

I used an older version of React so React.forwardRef is not working

[bvaughn]

is it possible to disable the default 'div' of outerElementType?

No. The two element structure is an important part of how react-window works.