New popover positioning service + updated EuiToolTip #924
Conversation
| @@ -58,7 +58,7 @@ module.exports = { | |||
|
|
|||
| devServer: { | |||
| contentBase: 'src-docs/build', | |||
| host: 'localhost', | |||
| host: '0.0.0.0', | |||
There was a problem hiding this comment.
this change allowed me to test from IE VM
There was a problem hiding this comment.
:( I wasn't able to get this to work in a Virtual Box
Just taking a first quick glance, does this mean if you send "top" it defaults to "topCenter" unless there isn't enough room to center and then it might be "topLeft" or "topRight"? This seems like appropriate behavior, but we'll then need to manipulate the positioning of the tooltip's arrow:
|
|
Hmm, it looks like it'll actually stick to the cross side when there's enough room on the explicit side. I'm not sure if this is the behavior we want, or if we need a way to find the side with the most room? I dunno, but here's an example where the tooltip alignment can look funky: |
There was a problem hiding this comment.
I did a feature leave test of this in IE/Chrome/FF/SAF. It works on all of them how I'd expect. I noticed some problems when the browser is resized, that go away after a couple hovers, but this is behavior in the current version and not a regression.
I'm going to leave the code review to my javascript betters as these calculations look deep.
I did at least scan the interface for usage and it reads easy enough to me from a consumer perspective.
If we assume the docs are coming with a fairly quick followup PR it's ok to avoid that for the moment (since it closes that very bad IE bug), but we will definitely want to add those in.
|
Also throwing this up here so I/we don't forget. We should make sure that we leave enough room for the pop in transition so we don't get horizontal scroll bars. |
|
Ok, hopefully this helps illustrate the idea of positioning the arrow with the tooltip. Since we already know the dimensions of the trigger element, all we need to do is position the arrow always center of the trigger element. Here's a couple extra guidelines that should help make in work in most use cases.
This also involves making the arrow slightly smaller. This can be achieved simply by changing the :pinocchio: |
@cchaos should that be a hard coded @snide two outstanding (and pre-existing) issues with the tooltips we've uncovered: 1. re-positioning the tooltip after resizing content 2. scrollbars jump a bit on some layouts ; I'd like to push addressing those to their own fast-follow PR. Thoughts? |
|
I think the 16px should be hard-coded. I can't see any reason why it would be necessary to make the tooltips closer to the edge of the window. Also that will fix your no. 2 issue of the scrollbars coming in during the animation. |
|
@cchaos Hard coding sounds good to me. It may fix some scroll bar issues, but there's sometimes a quick flash of vertical scroll bars changing as the content's added to the bottom of the page before being re-positioned. |
|
@chandlerprall missed your question from yesterday, but yeah, it's cool to cover them in a follow up. |
chandlerprall
force-pushed
the
feature/popover-service
branch
from
37ab4c5 to
07face1
Compare
|
Service now returns top/left values for the arrow, tooltip is refactored to use. @cchaos please take a look at the arrow size @cjcenizal I added more documentation to the code, everything should be good for a full look-through |
|
@chandlerprall I pushed a commit for ya that reduces the size of the arrow a bit. Looks like it still all looks good proximity wise. The one thing I noticed is that the proximity to the edge of the browser window doesn't take into mind the scrollbars, wah wah. But I'm ok if you need to push this to another PR.
And yeah after resizing the browser, it looks like the tooltip arrow is still being positioned correctly but the tooltip content is not.
|
…steps: 1) Find original cross-axis position. 2) Apply shift to cross-axis position, if necessary. 3) Find primary axis position.
| const containerBoundingBox = { top: 10, right: 90, bottom: 190, left: 25 }; | ||
|
|
||
| it('reports all empty space', () => { | ||
| expect(getAvailableSpace(anchorBoundingBox, containerBoundingBox, 0, 0, 'top')).toEqual({ |
There was a problem hiding this comment.
I was a little thrown off by the arguments for buffer, offset, and offsetSide because I couldn't see how they were relevant to the assertion being made. I think this test would be simpler if it was just calling getAvailableSpace(anchorBoundingBox, containerBoundingBox). We could do this by adding some defaults:
export function getAvailableSpace(anchorBoundingBox, containerBoundingBox, buffer = 0, offset = 0, offsetSide = undefined) {What do you think?
There was a problem hiding this comment.
I like keeping these as required arguments. It's an internal function and I think its interface should have some rigidity to force any implementers to think about the parameters,
| // 80h x 65w | ||
| const containerBoundingBox = { top: 10, right: 90, bottom: 190, left: 25 }; | ||
|
|
||
| it('reports all empty space', () => { |
There was a problem hiding this comment.
Can we make this more specific? E.g. returns the distance from each side of the anchor to each side of the container
| }); | ||
| }); | ||
|
|
||
| it('respects buffer', () => { |
There was a problem hiding this comment.
How about subtracts the buffer amount from the returned distances?
There was a problem hiding this comment.
And is offset and offsetSide pertinent here? If not, can we remove them?
| }); | ||
| }); | ||
|
|
||
| it('respects offset', () => { |
There was a problem hiding this comment.
How about subtracts the offset from the specified offsetSide?
If we want to cover all the bases, we could also iterate through all of the valid offsetSide values.
// We can reuse this object in other assertions too.
const expectedAvailableSpace = {
top: 90,
right: 30,
bottom: 40,
left: 5,
};
['top', 'right', 'bottom', 'left'].forEach(side => {
expect(getAvailableSpace(anchorBoundingBox, containerBoundingBox, 0, 5, side)).toEqual({
...expectedAvailableSpace,
[side]: expectedAvailableSpace[side] - 5,
});
});| }); | ||
| }); | ||
|
|
||
| it('respects buffer & offset', () => { |
There was a problem hiding this comment.
How about subtracts the buffer and the offset from the specified offsetSide?
| }); | ||
|
|
||
| describe('getVisibleFit', () => { | ||
| it('calculates full visibility when container is large', () => { |
There was a problem hiding this comment.
calculates full visibility when content is fully enclosed by container? And it might be more readable to use variables here:
const content = { top: 25, right: 50, bottom: 50, left: 25 };
const container = { top: 0, right: 500, bottom: 500, left: 0 };
expect(getVisibleFit(content, container).toBe(1);
Fixes #610
Popover positioning service
Purpose is to replace & enhance existing
calculatePopoverPositionEuiToolTip
Refactored to use the new service. This was a small change to allow passing the tooltip content node between React components instead of relying on EuiToolTipPopover to pass the client rect. Previous functionality is maintained to avoid a breaking change.
Todo after this PR