Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed tooltips for element links #12840

Merged
merged 6 commits into from
Dec 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 10 additions & 4 deletions packages/design-system/src/components/tooltip/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ function BaseTooltip({
[placement]
);

const getAnchor = useCallback(
() => forceAnchorRef || anchorRef,
[forceAnchorRef]
);

const positionPopup = useCallback(() => {
if (!isPopupMounted.current || !anchorRef?.current) {
return;
Expand All @@ -157,14 +162,14 @@ function BaseTooltip({
? getOffset({
placement: dynamicPlacement,
spacing,
anchor: forceAnchorRef || anchorRef,
anchor: getAnchor(),
popup,
isRTL,
ignoreMaxOffsetY: true,
})
: {},
});
}, [dynamicPlacement, spacing, forceAnchorRef, isRTL]);
}, [dynamicPlacement, spacing, getAnchor, isRTL]);

// When near the edge of the viewport we want to force the tooltip to a new placement as to not
// cutoff the contents of the tooltip.
Expand Down Expand Up @@ -210,7 +215,8 @@ function BaseTooltip({
);

const positionArrow = useCallback(() => {
const anchorElBoundingBox = anchorRef.current?.getBoundingClientRect();
const anchor = getAnchor();
const anchorElBoundingBox = anchor.current?.getBoundingClientRect();
const tooltipElBoundingBox = tooltipRef.current?.getBoundingClientRect();
if (!tooltipElBoundingBox || !anchorElBoundingBox) {
return;
Expand All @@ -222,7 +228,7 @@ function BaseTooltip({
getBoundingBoxCenter(tooltipElBoundingBox);

setArrowDelta(delta);
}, [positionPlacement, popupState]);
}, [positionPlacement, popupState, getAnchor]);

const resetPlacement = useDebouncedCallback(() => {
setDynamicPlacement(placementRef.current);
Expand Down
5 changes: 5 additions & 0 deletions packages/design-system/src/icons/external_link.svg
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions packages/design-system/src/icons/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export { default as DotsFillSmall } from './dots_fill_small.svg';
export { default as Eraser } from './eraser.svg';
export { default as ExclamationOutline } from './exclamation_outline.svg';
export { default as ExclamationTriangle } from './exclamation_triangle.svg';
export { default as ExternalLink } from './external_link.svg';
export { default as Eye } from './eye.svg';
export { default as FloppyDisk } from './floppy_disk.svg';
export { default as GearWithGauge } from './gear_with_gauge.svg';
Expand Down
23 changes: 15 additions & 8 deletions packages/story-editor/src/components/elementLink/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import styled from 'styled-components';
import { rgba } from 'polished';
import PropTypes from 'prop-types';
import { __ } from '@googleforcreators/i18n';
import { TOOLTIP_PLACEMENT } from '@googleforcreators/design-system';
import { withoutProtocol } from '@googleforcreators/url';
import { TOOLTIP_PLACEMENT, Icons } from '@googleforcreators/design-system';

/**
* Internal dependencies
Expand All @@ -40,11 +41,12 @@ const StyledTooltip = styled(Tooltip)`
}
`;

const IconWrapper = styled.div`
const IconWrapper = styled.span`
background-color: ${({ theme }) => theme.colors.fg.secondary};
width: 24px;
height: 24px;
border-radius: 50%;
overflow: hidden;
margin-right: 8px;
display: inline-block;
`;
Expand All @@ -64,30 +66,35 @@ const LinkDesc = styled.span`
overflow: hidden;
`;

const ExternalLink = styled(Icons.ExternalLink)`
width: 24px;
`;

function WithLink({ element, active, children, anchorRef }) {
const link = getLinkFromElement(element);

const tooltipContent =
link?.url && active ? (
<>
<IconWrapper>
{link?.icon && (
{link?.icon && (
<IconWrapper>
<BrandIcon
src={link.icon}
alt={__('Site Icon', 'web-stories')}
decoding="async"
crossOrigin="anonymous"
/>
)}
</IconWrapper>
<LinkDesc>{link.desc || link.url}</LinkDesc>
</IconWrapper>
)}
<LinkDesc>{link.desc || withoutProtocol(link.url)}</LinkDesc>
<ExternalLink />
</>
) : null;

return (
<StyledTooltip
forceAnchorRef={anchorRef}
placement={TOOLTIP_PLACEMENT.TOP_START}
placement={TOOLTIP_PLACEMENT.TOP}
title={tooltipContent}
>
{children}
Expand Down
22 changes: 12 additions & 10 deletions packages/url/src/test/url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
/**
* Internal dependencies
*/
import { withProtocol, isValidUrl } from '..';
import { withProtocol, isValidUrl, withoutProtocol } from '..';

describe('isValidUrl', () => {
it('should return correct results for valid URL', () => {
Expand All @@ -35,14 +35,16 @@ describe('isValidUrl', () => {

describe('withProtocol', () => {
it('should add protocol correctly if applicable', () => {
expect(withProtocol('https://foo-bar.com/test.jpg')).toBe(
'https://foo-bar.com/test.jpg'
);
expect(withProtocol('http://foo-bar.com/test.jpg')).toBe(
'http://foo-bar.com/test.jpg'
);
expect(withProtocol('foo-bar.com/test.jpg')).toBe(
'https://foo-bar.com/test.jpg'
);
expect(withProtocol('https://foo.com/test')).toBe('https://foo.com/test');
expect(withProtocol('http://foo.com/test')).toBe('http://foo.com/test');
expect(withProtocol('foo.com/test')).toBe('https://foo.com/test');
Comment on lines +38 to +40
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason for this change is to make each fit on a single line because it's prettier 😂

});
});

describe('withoutProtocol', () => {
it('should remove protocol correctly if applicable', () => {
expect(withoutProtocol('https://foo.com/test')).toBe('foo.com/test');
expect(withoutProtocol('http://foo.com/test')).toBe('foo.com/test');
expect(withoutProtocol('foo.com/test')).toBe('foo.com/test');
});
});
4 changes: 4 additions & 0 deletions packages/url/src/url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,7 @@ export function withProtocol(
? url
: `${protocol}://${url}`;
}

export function withoutProtocol(url: string): string {
return url.replace(/^(http:\/\/|https:\/\/|tel:|mailto:)/, '');
barklund marked this conversation as resolved.
Show resolved Hide resolved
}