feat(Label) add option to make label clickable

[Dominik-Petrik]

What: Closes #8665

I used onLabelClick instead of onClick as a prop name because of name conflict. I think maybe a better prop name could be used but I am not sure what it should be.

[Dominik-Petrik]

One more thought that popped up is maybe we want to differentiate visually between clickable and link labels, so the user can expect what happens on clicking a label.

[mmenestr]

I don't know that we ever recommend using labels as links. Do you know of any product doing this?

[patternfly-build]

Preview: https://patternfly-react-pr-9284.surge.sh

A11y report: https://patternfly-react-pr-9284-a11y.surge.sh

[mcoker]

LGTM! Just one nit about needing an explicit type="button" on the <button> element

[mcoker]

this is going to need type="button" if it's a <button>, since a button's default type is "submit", which will submit a form if it's placed in a form and you click on it.

[mcoker]

@mmenestr from the original issue when the feature was added, the main use case was for labels as links. Some info in the issue - patternfly/patternfly#2656 (comment), and in the PR - patternfly/patternfly#2943 (comment)

cc @mcarrano

[Dominik-Petrik]

I don't know that we ever recommend using labels as links. Do you know of any product doing this?

@mmenestr It seems like we kinda recommend using labels as links in our design guidelines here. I am not aware of any product that uses them right now.

[mcarrano]

@Dominik-Petrik @mmenestr Unfortunately I can't recall why we decided that labels could be used for links but not as buttons. It may have been that there was a use case to click on a label to reveal more information. But I case also see the case for clicking a label to apply a filter, for example. @maryshak1996 looks like you worked on this 3 years ago. Do you have any memory of this?

I do think that allowing for either a link or a button action makes sense, especially since the design guidelines seem to support that. Whether there needs to be a visual difference, I'm not sure. We could underline the text on hover for links but the distinction might not be important to users IMO. @andrew-ronaldson would also be interested to hear your thoughts.

[mmenestr]

Oh, you're right @Dominik-Petrik I guess I was thinking we shouldn't use labels for external links - I think that would be weird. But it's true that there are other types of links.

Idk that we need to differentiate between the two. We had done a study a while ago (though not on labels) about whether users care to have different styles for different actions/link types, and the conclusion was basically that it didn't really matter as long as it was clear that the thing was clickable. In other words, whether it's a blue link, or a underlined link or some other visual styling didn't matter to users, even if the actions differed.

[Dominik-Petrik]

Cool, thanks for clarification!

[thatblindgeye]

Could we also provide a console warning for if both isOverflowLabel and onLabelClick are passed? Doing so causes a button to render inside a button which isn't valid.

There might be other combinations of props that would be good to have a console warning for as well, but we can open a followup if necessary

[thatblindgeye]

It could make sense to only pass on onLabelClick if the LabelComponentChildElement === 'button', as we probably don't want to allow a label to both trigger an action on click and redirect a user to another page. That would also mean updating prop descriptions to mention not using href if onLabelClick is passed and vice versa

[wise-king-sullyman]

+1

Also I think having this logic as part of the labelComponentChildProps definition would be a bit cleaner of an approach.

[thatblindgeye]

Nit: "Callback for when the label is clicked." We could omit the "label will be clickable" portion I think,

Or per my above comment, "Callback for when the label is clicked. This should not be passed in if the href prop is also passed in." (the href prop could just have "This should not be passed in if the onLabelClick prop is also passed in."

[kmcfaul]

+1

[wise-king-sullyman]

+1 from me as well. Also what name conflict forces us to not use onClick as the prop name? I think that's the prop name I would expect for this as a consumer.

[mcoker]

LGTM 🚀

[wise-king-sullyman]

+1

Also I think having this logic as part of the labelComponentChildProps definition would be a bit cleaner of an approach.

[wise-king-sullyman]

Could you edit this test name to be a bit more descriptive? I.e. "a button is rendered when x is passed" or something along those lines.

Also I would like to see some "negative" tests here that assert things work as we expect in the opposite cases. For instance that your screen.getByRole(button) isn't visible (actually I think you would have to just assert that it's .not.toBeInTheDocument() IIRC) when onLabelClick isn't passed, and that the callback isn't called when it's passed but the label isn't clicked.

[andrew-ronaldson]

LGTM, thanks!

[wise-king-sullyman]

I'll try to get this updated so that we can get it into this release.

[thatblindgeye]

Since we're now passing the onClick prop in different places depending on if other props are passed, isOverflowLabel and onClick should now both be valid to pass in together, otherwise the LabelGroup code would technically be invalid since we're passing both props into an overflow label:

patternfly-react/packages/react-core/src/components/Label/LabelGroup.tsx

Lines 183 to 193 in 2fa883d

	{numChildren > numLabels && (
	<li className={css(styles.labelGroupListItem)}>
	<Label
	isOverflowLabel
	onClick={this.toggleCollapse}
	className={css(isCompact && labelStyles.modifiers.compact)}
	>
	{isOpen ? expandedText : collapsedTextResult}
	</Label>
	</li>
	)}

[thatblindgeye]

This can be reverted now

[wise-king-sullyman]

Great catch on these

[thatblindgeye]

Suggested change

	/** Callback for when the label is clicked. This should not be passed in if the href or isOverflowLabel props are also passed in. */
	/** Callback for when the label is clicked. This should not be passed in if the href prop is also passed in. */

[wise-king-sullyman]

This should probably be changed to or isEditable prop right?

[thatblindgeye]

Oh yep, should be a swap instead of a removal

[thatblindgeye]

We can remove the first if block/mentions of isOverflowLabel from this useEffect

[tlabaj]

lgtm

[gitdallas]

There are ways to create mutually exclusive properties in types, although I'm not sure we'd want to go that way and how it might make the doc generation look.