LPS-128506 - Make sure taglibs using react and clay are using proper tooltip #855
Conversation
|
CI is automatically triggering the following test suites:
|
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-128506 1 Successful Jobs:For more details click here. |
|
I added an additional commit removing uses of ClayTooltipProvider. I haven't fully tested all of these instances yet |
|
Jenkins Build:test-portal-source-format#6250 |
| @@ -38,6 +38,8 @@ const ALIGN_POSITIONS = [ | |||
|
|
|||
| const SELECTOR_TOOLTIP = '.tooltip[role="tooltip"]'; | |||
| const SELECTOR_TRIGGER = ` | |||
| .taglib-clay-tag [title], | |||
| .taglib-clay-tag [data-restore-title], | |||
There was a problem hiding this comment.
I think maybe we finally want to be more aggressive and just do [title], [data-title] as a trigger selector and clean up all the specific exceptions we've been adding over time instead of adding a new one.
I don't think the [data-restore-title] selector should really be used even though it appears in other entries below. It basically signals an intermediate step where we usually move the title attribute to data-restore-title so we can leave html the same way we found it before showing a tooltip and avoiding a double tooltip triggered by the default behaviour of a title attribute.
There was a problem hiding this comment.
[date-restore-title] was necessary for the mouseout event and removing the tooltip, we use the same selector list for both enabling and disabling the tooltip. But I think you are right, we should just do all [title] and [data-title] and then we can add an explicit opt-out such as :not([data-disable-tooltip])
| <ClayIconSpriteContext.Provider value={spritemap}> | ||
| {Component ? <Component {...renderData} /> : renderable} | ||
| </ClayIconSpriteContext.Provider>, | ||
| <ClayTooltipProvider> |
There was a problem hiding this comment.
This looks good to me. I was just wondering if @matuzalemsteles or @wincent can think of a reason we decided not to do this yet. Maybe it just didn't come up.
There was a problem hiding this comment.
Yep, as far as I know, we never even discussed this possibility, but it seems fine to do it.
There was a problem hiding this comment.
Well, I think the initial idea of this was to be the Provider of the application but probably people were always adding above the component with Title for some reason... I remember we had an issue open in Clay where we would centralize all Providers, something like ClayManager to probably add to the render. This would contain:
- ClayModalProvider
- ClayIconSpriteContext
- ClayTooltipProvider
I think we didn't get to prioritize that because we haven’t seen a great need at the moment. This could alleviate forgetting to add any Provider.
Perhaps another reason was that Tooltip was also always controlled by singleton on the Portal and we had been working on his evolution to use React, maybe that discouraged us from using Provider since it only works on the React tree.
| @@ -15,7 +15,6 @@ | |||
| import {ClayButtonWithIcon} from '@clayui/button'; | |||
| import ClayForm, {ClayRadio, ClayRadioGroup, ClayToggle} from '@clayui/form'; | |||
| import ClayPopover from '@clayui/popover'; | |||
| import {ClayTooltipProvider} from '@clayui/tooltip'; | |||
There was a problem hiding this comment.
I'd expect at least some removals in package.json dependencies of @clayui/tooltip that are surely unnecessary after this.
There was a problem hiding this comment.
Very true, didn't think about that. I'll update
❌ ci:test:stable - 8 out of 9 jobs passed❌ ci:test:relevant - 17 out of 23 jobs passed in 3 hours 31 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: d157dfa16ee97ddd0425114fbc41f621b01cea40 ci:test:stable - 8 out of 9 jobs PASSED1 Failed Jobs:8 Successful Jobs:
ci:test:relevant - 17 out of 23 jobs PASSED6 Failed Jobs:
17 Successful Jobs:
For more details click here.Failures unique to this pull:
Failures in common with acceptance upstream results at d157dfa:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#7864 |
bryceosterhaus
force-pushed
the
LPS-128506
branch
from
cf865a3 to
08e41a7
Compare
|
Taking this out of |
|
As much as I like this PR, I'm not confident this works as intended <ClayTooltipProvider>
<ClayMultiSelect />
</ClayTooltipProvider>This apparently only works for the main input element but not for the rest of buttons or components. I assume because @bryceosterhaus also tweaked the tags to work with our global tooltip provider that he saw it working, but I think that's a false positive since this should work out of the box and it does not. <ClayTooltipProvider>
<div>
<ClayMultiSelect />
</div>
</ClayTooltipProvider>This will actually also make the tooltip over the button show up. I've created this codesandbox where this behaviour can be seen. Note that even though the "Clear All" button is not visible, it's there and hovering over it shows or not the tooltip while hovering over the input always shows the tooltip. I'm going to open an issue in Clay now and return this back to /cc @matuzalemsteles, @wincent |
|
@jbalsas I think this probably won't work for this case because it will depend on Maybe we can make this wrapper in TooltipProvider or add it to the render? |
Hmmm... I see, the thing is we even wrap that further but apparently following the same approach in our DXP Multiselect component wrapper What would be your proposal to address this? :D
I'm worried about adding unnecessary wrappers since that can easily break layouts, so we should find a way to easily target only the needed components. One thing I'm also worried about now that I'm seeing the code is this: <ClayTooltipProvider>
<ClayButton
title="hey!"
onMouseOver={() => {alert('foo');}}
onMouseOut={() => {alert('bar');}}
>
Button with tooltip but not mouse events
</ClayButton>
</ClayTooltipProvider>I've updated the codesandbox sample to show this in 2 different buttons, so How do you think we should proceed updating either /cc @wincent |
Yeah, I totally agree I also don't like adding divs to handle this, it creates a mess in the markup and will eventually break the layout.
Yeah, that can be fixed to still allow the developer to hear those events and Clay too.
Well, the
As |
|
Hey @matuzalemsteles, I just thought that since this was already being done like that, we could simply add an additional This obviously doesn't fix any of the issues we've discussed, but maybe it's a simpler pattern. If the tooltip provider doesn't work because of the implementation of your component, you have to either provide a wrapper or another TooltipProvider in a place you know I think this at least allows us to unblock this and provide tooltips by default in react in an easier way for the rest of components. Thoughts on this anyone? |
In this case, this is necessary because of how the component is passing `otherProps` to a specific element within the element hierarchy and not directly to a container. This makes the mouse listeners to be added to the wrong element and not triggered in other parts of the component. To solve this in this case we can simply add one additional TooltipProvider which targets an ancestor wrapper and can handle the mouse interactions properly
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-128506 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#4414 |
|
Reviewed. Test failures are unrelated. ✔️ [Unrelated]
|
|
Hi @john-co , this test is actually related to my team. I'll take a look. Thanks! |
|
I'd also consider the last run unique test failure on modules-integration-mysql57-jdk8/0 as unrelated. Will manually forward. |
|
forwarded to brianchandotcom#99837 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#7024 |
This addresses https://issues.liferay.com/browse/LPS-128506
The main change here is that we wrap out
renderwith aClayTooltipProvidersimilar to how we provide aClayIconContextso that devs don't have to provide it themselves(unless they want to override the styles).This means that everything render through
react:component,ReactRendererorimport {render} from 'frontend-js-react-web' automatically provides default tooltip support simplifying the setup for applications down the line.Because removing the provider in the modules changes basically all the indentation of the files, it can easily cause conflicts.
/cc @liferay-echo, @liferay-tango, @liferay-lima, @liferay-commerce, @liferay-headless, @liferay-app-builder, @liferay-data-engine, @liferay-forms, @liferay-workflow, @liferay-search... and maybe others 🙈