fix(DataList): Remove redundant while

[karelhala]

There is while that does nothing just stalls the CPU when user clicks on any data list item. This PR removes such while since it's not needed.

[nicolethoen]

@karelhala This while loop is there to check that the user is intending to select a DataListItem rather than interact with a child component with a different click hander. As long as you have tested that clicking any checkboxes or dropdowns or any other interact-able child components in the dataListItem does not select the row, then the while loop is unnecessary. When it is done building I will test this as well.

[patternfly-build]

PatternFly-React preview: https://patternfly-react-pr-3449.surge.sh

[karelhala]

As long as you have tested that clicking any checkboxes or dropdowns or any other interact-able child components in the dataListItem does not select the row, then the while loop is unnecessary

Well actually that is what should it should be doing. If some element has onClick function on it, consumer usually expects that clicking anywhere in this element will fire such action. If I don't want to select row when clicking on some specific element I can add stopPropagation function.

Nevertheless I understand why it is there, but while function for this is not really good way. The prefered way should be using ref and checking if target is DataListItem.

[Hyperkid123]

@nicolethoen if that is the case, the dev should use event.stopPropagation. This loop is not safe and when used outside of data list will break.

[nicolethoen]

Well actually that is what should it should be doing.

@karelhala preventing this behavior was an explicit requirement of the enhancement. But I am happy to work with you to find a better way to implement it. I had to consult with a number of react devs to get to this point. So i'd be very open to other ideas.

and when used outside of data list will break.

@Hyperkid123 but it's internal to the DataListItem component - which is designed to only be used in DataList, so I'm not sure what you mean here.

[Hyperkid123]

@nicolethoen I don't see any point for a loop like that. Using stopPropagation has been common practice in JS from start and React is no different (it's just a JS library).

Here is a quick demo: https://codesandbox.io/s/gifted-monad-q6d9x

If there are any composite data-list component they should just kill the event.

[nicolethoen]

@Hyperkid123
It would put the responsibility of handling event propagation in the hands of the consumer. The requirements wanted it built into the component. So if there is a way to bake stopPropagation into the DataListItem component itself (I was unable to get it working since I cannot control which kinds of child components are passed to the dataListItem), then I'd be very open to that.

[Hyperkid123]

It would put the responsibility of handling event propagation in the hands of the consumer.

And this is a problem why? What if the consumer wants to propagate the event? PF should not be some magical thing that will do everything for us. It should be a set of building blocks.

Can we get a response from the person who wanted it like this? I understand that this probably was not your decision.

[nicolethoen]

And this is a problem why? What if the consumer wants to propagate the event? PF should not be some magical thigh that will do everything for use. It should be a set of building blocks.

This discussion is larger, then. The interaction designers decided it was important that stopping the event propagation was not optional and was enforced by the component. Some of the conversation happened offline, but you can see a reference to the debate in the original PR comments.

#3404 (comment)

@mcarrano @LHinson @rachael-phillips

[Hyperkid123]

I am not against the design. I don't mind the catching of the event. I don't like the implementation. Plus not all events are triggered by clicking.

@mcarrano using the loop is not safe and using stop propagation on child components/elements will bring more customization and performance increase.

[nicolethoen]

@Hyperkid123 Feel free to propose an alternative code change :) We just cannot remove the functionality all together (without changing the requirements).

[karelhala]

For dataList items that is understandable, if I click on kebab the row should not be selected that is true. However if you use button, input, w/e with this component and add onclick function to it the consumer should be responsible for blocking events.

I see that the initial comment is from @mcarrano let me elaborate on this. If you add input in this component with this while and if user clicks in the text input the row is still selected. If there is no checkbox we have no other way how to properly prevent selecting rows. We simply can't go trough the DOM and check if some element is active and can triggier click event. The if would be super complex and bug prone. If the design has blue stripe for selected rows instead of checkbox we have to allow clicking anywhere in row to trigger select. We have to send event alongside the id so consumer knows when to select and when not to select. By default me as a consumer I would write preventDefault for elements that I don't want to trigger select, or I would check if user clicked on BUTTON or INPUT.

We can't limit consumers in their actions, this is the reason why so many people are unhappy with PF. Whenever they use some component it is usable for simple and straight forward usecase, but once they want to use it for something complex it just won't work or make their work really difficult.

I am willing to change this PR to "block" events in DataListAction and DataListControl.

[mcarrano]

@karelhala @Hyperkid123 @nicolethoen I am not opposed to using the technique suggested here to stop propagation of the event. My only requirements are that it be possible to block an event triggered from a child component (like opening the kabob menu) from selecting a row AND that the example clearly illustrates how to do that.

@karelhala I hear your point about giving consumers of PF greater flexibility in coding behaviors related to these components. I think this is a larger conversation to be had about what the appropriate balance is and how to allow that flexibility while driving consistency in user experience between applications. @tlabaj @dgutride any thoughts?

[redallen]

Much cleaner.

Edit: Failing jest_test_other can be safely ignored.

[jschuler]

@karelhala @nicolethoen @Hyperkid123 I have an idea to preserve the original intent which was to prevent actionable items like buttons, dropdowns, from selecting a row. Actionable items are wrapped in the DataListAction component, so we could simply add a new prop like stopPropagation?: boolean; to it, and also attach a click event listener here so it can intercept children's events if needed.

const catchBubble = (event: React.MouseEvent) => {
      if (props.stopPropagation) {
        console.log('stopped event from bubbling up');
        event.stopPropagation();
        event.nativeEvent.stopImmediatePropagation();
      }
    }

    return (
      <div onClick={catchBubble} className={css(styles.dataListItemAction, className)} {...props}>
        {children}
      </div>
    );

[karelhala]

@jschuler yeah, I like that approach. How about we combine what you proposed (adding a prop to ignore clicks on children) and at the same time we hide this while behind same prop. So we give consumers a bit of more flexibility on this issue and expect they know what are doing when adding an element with onClick attached to it. But at the same time if they want to ignore all clicks we give them this option, perhaps mark it in the props that this option can have impact on performance.

[karelhala]

I don't really have time to investigate this PR further. Closing for now and I might come to this later.