Add HoverText UI Component

[tn3rb]

Problem this Pull Request solves

easy consistent way to add a "popup" help text and/or description to a component, button, link, icon, etc

How has this been tested

includes unit tests and currently in use in the REM add-on

Checklist

• I have added a changelog entry for this pull request
• I have read the documentation relating to systems affected by this pull request, see https://github.com/eventespresso/event-espresso-core/tree/master/docs
• User input is adequately validated and sanitized
• all publicly displayed strings are internationalized (usually using esc_html__(), see https://codex.wordpress.org/I18n_for_WordPress_Developers)
• My code is tested.
• My code follows the Event Espresso code style.
• My code has proper inline documentation.
• My code accounts for when the site is Maintenance Mode (MM2 especially disallows usage of models)

[nerrad]

I'm curious about why you decided to create this instead of using the Popover or Modal components provided by GB? If you decide to still go with this:

• manually controlling the hover component is nice (and should be kept) but with it ONLY being manually controlled its not very responsive. The Popover component in GB is responsive and automatically calculates if it can be displayed in the requested position and adjusts its position of necessary.
• Has this been tested on mobile? It doesn't appear to be very mobile friendly.
• I think it'd be good to accept some event handler props for things like clicking outside of the popover or when the popover is closed.
• Accessibility wise, it'd be good to allow for usage of the ESC key to dismiss the hovertext.
• I also like how the Popover GB element allows for manually designating where a popover appears in the rendered UI using Slot/Fill. Something similar here would be useful.

[nerrad]

There's potential for non unique ids to be implemented if there are multiple instances of this component in use. Granted it would mean that the developer would have to provide the same htmlId and description for this to happen but its still possible. I think it'd be good to wrap this with the withInstanceId HOC so the id is guaranteed to be unique.

[tn3rb]

I can do that, but it's not actually critical that this has a totally unique id as we are not using that for any kind of targeting. It's really only needed if a dev wants to tweak the styling for a specific hover text in a component, in which case, the dev can simply ensure that it has a unique id, by passing one.

The withInstanceId HOC was necessary in Gutenberg because of the way blocks and the GB admin works which absolutely requires unique ids in order to function properly, otherwise performing an action like deleting a block could affect more than one instance. For our purposes we are not using ids for control like that, and would only ever need it for styling purposes (remember this is just hover text) so having an id that potentially changes due to the number of calls to withInstanceId would actually be counter to our purposes.

[nerrad]

So its okay to have potentially invalid html rendered?

[nerrad]

If an html id attribute is not needed then I would say don't bother adding it.

[nerrad]

would actually be counter to our purposes.

What purposes would it be counter to?

[nerrad]

If that's the case, and you decide to keep this component, I'd prefer it if you either:

• remove the id and developers can tweak styling by the htmlClass prop. I don't see how having an id is necessary if they can provide a className. If an id is necessary for styling then that is argument for it being unique.
  OR
• wrap the component with withInstanceId and implement so the html id attribute is unique.

However, my preference is that you just use PopOver component from @wordpress/components because from what I've seen so far, I don't see the need for re-inventing the wheel here.

[tn3rb]

So its okay to have potentially invalid html rendered?

No you are making a strawman argument as that was not my point. A form input for example can still work correctly even if a developer fails to give it a unique id. That does not mean that invalid html is ok, it only means that nothing breaks as a result of that invalid html. With Gutenberg blocks, it is critical that some components have a unique id else the editor will not be able to target them properly and things will not work.

I don't see how having an id is necessary if they can provide a className.

class names are not intended to be unique so if you have multiple elements that share the same class, then you need to start relying on more complex css to target a specific element, which may not always be possible.

If an id is necessary for styling then that is argument for it being unique.

There is nothing preventing the htmlId prop here from being unique. It CAN be unique simply by setting a unique value when adding the component to your app. This is no different than our PHP form system that generates ids for form elements in that it is up to the developer to ensure that the data they are inputting results in valid html. If i had two inputs in a row in a form and used "my-input" for the id for both inputs, then that would result in invalid html but it would NOT prevent the form from operating correctly in any other way, as long as I did not require those input ids for any kind of targeting (JS or CSS). Having a duplicate id for some hover text is not good, but it will not break anything.

With Gutenberg blocks it is a totally different scenario because the editor can create multiple instances of a block dynamically, so I could add one, two, three... ten... one hundred... (one miiiillllliiiion) copies of the same block to a post, and in order for GB to be able to target each one of those instances separately, they all need to have an id that is at least partially dynamically generated. This can also apply to components being used within the block, especially if you are adding form elements to the Gutenberg sidebar, because again, those may need to have a unique id for targeting purposes.

Here is the first sentence from the documentation for the withInstanceId HOC:

Some components need to generate a unique id for each instance.

There is nothing in the documentation stating that every single component that implements an id must absolutely use the withInstanceId HOC in order to guarantee the uniqueness of that id. But SOME components do need to generate a unique id for each instance. To suggest that a non-critical component like some hover text needs to utilize the withInstanceId HOC in order to guarantee a totally unique id attribute is totally overboard. If it is that much of an issue, then the more sane approach would be to use the withInstanceId HOC on a more appropriate parent component higher up in the hierarchy of your app, and then pass that id down to any child elements and have them incorporate that into any ids that they set. In other words, there is no reason why the html id for some hover text could not incorporate a unique id that was passed down from a withInstanceId HOC that was used higher up in the app.

[nerrad]

I understand how withInstanceId works and its purpose. I'm not making a straw man argument. My point is that IF someone was to generate a bunch of these components without passing in a unique htmlId prop (and other props used for the id are the same), then the result would be invalid html rendered. I'm not saying it won't work, I'm saying it would be invalid. So my question was simply asking "So its okay to have potentially invalid html rendered?" (note I emphasized potentially here). Your answer can simply be that you believe it is.

I still stand by my outlined preferences here.

[tn3rb]

There is nothing stopping this html id prop from being unique, but the dev implementing it in their component would be responsible for actually generating a unique id in the first place, and that is no different than a bunch of our other systems that we already have in place (like our form system).

Again, the sane thing to do would be to use withInstanceId on a more appropriate parent component in any app that can be implemented more than once on the same page, and then pass that unique id down through the component hierarchy. In other words, only use the withInstanceId HOC for things that can have multiple instances generated dynamically. It's total overkill to employ it every single time you plan to implement an html id attribute.

[nerrad]

I understand that. However, that's why one of the options I gave was:

remove the id and developers can tweak styling by the htmlClass prop. I don't see how having an id is necessary if they can provide a className. If an id is necessary for styling then that is argument for it being unique.

Developers can pass along a unique className for the css class if they want. In other words don't set an id if it's not necessary.

Now you didn't mention it, but just in case its one possible reason you are explicitly adding an id here (beyond for styling purposes), if the id is something you're adding so the element can be targeted by js, it'd be better if you wrap the component with React.forwardRef and set a ref on the element you want targeted as that's the recommended way to integrate custom binding to rendered elements.

[tn3rb]

I'm curious about why you decided to create this instead of using the Popover or Modal components provided by GB?

I missed the Popover component simply because of its name and didn't realize it was tooltip like. I think I assumed it was some kind of popup. It also appears to be triggered by clicking as opposed to hovering, which is not I want at all. BUt I can check it out.

I looked in the GB components for a Modal window when I started on the the REM stuff but did not see anything. I must have just missed it by a hair as it appears the Modal component you linked to was only merged in 17 days ago ( WordPress/gutenberg#6261 (comment) ) and I may have not pulled it into my local repo for a few days as well. I can check that out to see how it works.

[nerrad]

It also appears to be triggered by clicking as opposed to hovering, which is not I want at all.

No, its ambivalent about how its triggered. So you could have a parent component implement it by checking for the onMouseOver event and if present it can set something like showPopOver and that determines whether the <PopOver /> component is rendered or not. So some pseudo code:

import { Popover } from '@wordpress/components';

class MyComponentWithPopover extends Component {
	state = {
		showPopOver: false,
	};

	toggleVisible = () => {
		this.setState( { showPopOver: true } );
	}

	render() {
		return (
			<div onMouseOver={ this.toggleVisible }>
				{ this.props.children }
				{ this.state.showPopOver && (
					<PopOver
						onClose={ this.toggleVisible }
					>
						{ this.props.toolTipContent }
					</PopOver>
				) }
			</div>
		)
	};
}

[nerrad]

please note I edited my above comment if you see this in email notifications (edited to add onMouseOver)

[tn3rb]

Adding state to every component that wants to have an element with hover text is ridiculous imho. What happens if you have a component that has multiple elements that require hover text? You'd have to add state and separate callbacks for each element. Completely unnecessary. The benefit of my approach is that it is using css :hover to trigger the display, so you only have to import the component and wrap the element you want to add hover text to.

I'll check out the Popover component though, and maybe use it within the HoverText component so that parent components don't have to manage the hover state themselves. Maybe this is what you meant by your example, but that's not what it looks like to me.

[nerrad]

I'll check out the Popover component though, and maybe use it within the HoverText component so that parent components don't have to manage the hover state themselves. Maybe this is what you meant by your example, but that's not what it looks like to me.

That was the intent behind the code example. I'm glad you thought that might be what I was giving as an example. I tried to make it clear that what I was giving was pseudo code, I was merely trying to give an example how the Popover component could be implemented - I wouldn't expect you to do that for anything needing a Popover. So yes, I'd expect you'd create some sort of component like HoverText that implements Popover.

[nerrad]

There's also the WP tooltip component that might accomplish your needs as well. And actually ToolTip is an implementation of Popover with all the necessary handling (similar to what you're doing with HoverText). No need to re-invent the wheel here.

[tn3rb]

Ya I saw ToolTip when I went in to take a closer look at Popover.

So one problem with ToolTip is that... well here's the error message it's dumping:

Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail.

and then it just doesn't work...

so looks like I have to convert other things over to being actual React Components and can't use anything purely functional, which kinda stinks.

[tn3rb]

killing this in favour of the Gutenberg Tooltip component even though that appears to have some issues... will need to confirm that though