cc @jjenzz @chaance @peduarte

I'm really liking this idea, it'll be really useful to help educate people on the accessible/semantic ways of doings things when they try to change stuff.

My initial thought was that if we know they've changed the node then we can add the relevant attributes for them but that will make our side quite bloated, especially for those that don't change anything. I like how this keeps things lean and educates all at the same time.

On one hand I agree this could be very helpful, but on the other hand I could see it being extremely annoying if you have a legitimate use case for diverging from the default. Would we have any way to (intelligently) allow users to bypass these warnings if they've properly shimmed missing behaviors?

I was expecting someone would say that haha.
We could have a prop to quiet the warning but then it's almost too easy to turn it off.
Although we could say that if they turn it off, they should have read it…

but we all know devs don't read warnings…

@benoitgrelard Exactly my dilemma haha 🙃

In Reach UI we use dev warnings for when consumers don't import styles. There's no CLI option to disable them but you can easily turn them off by setting a CSS custom property on :root. Amazingly I still got sooo many issues and complaints about that.

In the end some folks are always going to misuse our components, and there's absolutely nothing we can do to stop them. Someone will use a menu button when they should have used a select.

One thought: maybe we only throw these warnings in the browser, and we only show a warning once per component. That way tests and CI logs don't get cluttered up, which I think is what bugs most folks. Browser console warnings are pretty easy to ignore if they know they are only shown during development.

but we couldn't always only show it for one, because it could differ at the each use case level, via as for example.

@benoitgrelard true but I think that's unlikely. I imagine in practice, most folks are going to wrap our component once and use it in their design system or app the same way everywhere. But my assumption may very well be wrong.

FWIW I don't care if we annoy the hell out of people if they are actually doing it wrong, haha. It's just tricky to detect when they may have taken the extra steps to do it right.

Yes probably, but there are still plenty of places where it's done at the use case levels, things like Dialog.Trigger, Popover.Trigger, Tooltip.Trigger, etc

FWIW I don't care if we annoy the hell out of people if they are actually doing it wrong, haha. It's just tricky to detect when they may have taken the extra steps to do it right.

Yeah I don't think we should even attempt to detect anything. I'd be fine with providing a muteTagWarning prop or whatever as they would actively have to put it in… We've done our job at this point.

It can be one of those extremely long and annoying one like: __dangerouslyIgnoreTagWarning or __signUpWithMyLifeThatIHaveReadTheConditionsAndKnowTheConsequences.

_IF_YOU_IGNORE_THIS_WARNING_YOU_ARE_A_BAD_PERSON_PROBABLY

Yeah, that's a very good point. Hmm... I'm gonna have a think on this more.

Perhaps, instead of logging warnings we can call an onWarning callback. Then we can document how to use it for those that care about this stuff:

function DialogTrigger = (props) => {
	// if someone messes up dialog triggers we log to our logging system 
	// because we care about their accessibility/semantics
	return (
		<Dialog.Trigger 
			{...props}
			onWarning={composeEventHandlers(props.onWarning, (event) => {
				myLoggingTool.log(event.detail);
			})}
		/>
	);
}

We could maybe even provide a DevWarnings component that they can wrap their app with to opt in, that will receive the bubbled dev warning events?

<DevWarnings onWarning={(event) => myLoggingTool.log(event.detail)} />

// I know I've done this right
<MyDialogTrigger onWarning={event => event.stopPropagation()} />

🤷

I like the idea of this in conjunction with tiny-invariant to error. You can also document an environment variable that people can use to convert the errors into warnings.

Just ran into this when a developer used a Radix tooltip component abstraction with a div that ended up applying type="button" and caused a style regression on Safari, but functionally "worked" (albeit inaccessibly).

More thoughts on dev warnings...

It can sometimes be confusing from a consumer perspective to do something like the following and end up with a null ref:

const App = () => {
	const dropdownContentRef = React.useRef();

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

	return (
		<DropdownMenu.Root>
			<DropdownMenu.Trigger>Open</DropdownMenu.Trigger>
			<DropdownMenu.Content ref={dropdownContentRef}>
				<DropdownMenu.Item>One</DropdownMenu.Item>
				<DropdownMenu.Item>Two</DropdownMenu.Item>
				<DropdownMenu.Item>Three</DropdownMenu.Item>
			</DropdownMenu.Content>
		</DropdownMenu.Root>
	);
}

https://codesandbox.io/s/inspiring-saha-742wm?file=/src/App.js

This looks/feels like it might work because the consumer has mounted the DropdownMenu.Content but internally we haven't mounted the DOM node so it wouldn't.

These sorts of things aren't obvious. Dev warnings would help us communicate that a callback ref would be more apt in this scenario for parts that conditionally render their DOM node:

const App = () => {
	const [dropdownContent, setDropdownContent] = React.useState();

	React.useEffect(() => {
		console.log(dropdownContent);
	}, [dropdownContent]);

	return (
		<DropdownMenu.Root>
			<DropdownMenu.Trigger>Open</DropdownMenu.Trigger>
			<DropdownMenu.Content ref={setDropdownContent}>
				<DropdownMenu.Item>One</DropdownMenu.Item>
				<DropdownMenu.Item>Two</DropdownMenu.Item>
				<DropdownMenu.Item>Three</DropdownMenu.Item>
			</DropdownMenu.Content>
		</DropdownMenu.Root>
	);
}

https://codesandbox.io/s/cocky-cori-cl10d?file=/src/App.js