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

Move tooltips to the design package #49544

Merged
merged 10 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what's the difference between HoverTooltip and ToolTipInfo? They seem to behave identical in the story 🤔 Maybe we should remove or deprecate one of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They behave in a similar, if not identical manner, but they have different interfaces, because ToolTipInfo is a specialized case for displaying icons with tooltips. It's used in places like field labels. I was there once, when I was working on modernizing these, and I also had a brief reflex to unify them. To my surprise, the result of my unification attempt was uglier and more complex than the original, and required a lot of refactoring of its usage, so I decided to keep these two separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would you think of renaming TooltipInfo to IconTooltip then? I'm not sure if Info provides any value.
But if it's too much for this PR we can leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a good idea. I'll do it.

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import React, { PropsWithChildren, useState } from 'react';
import styled, { useTheme } from 'styled-components';

import { Popover, Flex, Text } from 'design';
import { JustifyContentProps, FlexBasisProps } from 'design/system';

Expand Down Expand Up @@ -64,6 +65,7 @@ export const HoverTooltip: React.FC<
// whether we want to show the tooltip.
if (
target instanceof Element &&
target.parentElement &&
target.scrollWidth > target.parentElement.offsetWidth
) {
setAnchorEl(event.currentTarget);
Expand All @@ -75,7 +77,7 @@ export const HoverTooltip: React.FC<
}

function handlePopoverClose() {
setAnchorEl(null);
setAnchorEl(undefined);
}

// Don't render the tooltip if the content is undefined.
Expand Down
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
*/

import React from 'react';
import { Text, Flex, ButtonPrimary } from 'design';

import styled, { useTheme } from 'styled-components';

import { Text, Flex, ButtonPrimary } from 'design';
import { P } from 'design/Text/Text';
import { logos } from 'teleport/components/LogoHero/LogoHero';
import AGPLLogoLight from 'design/assets/images/agpl-light.svg';
import AGPLLogoDark from 'design/assets/images/agpl-dark.svg';

import { ToolTipInfo } from './ToolTip';
import { HoverTooltip } from './HoverTooltip';
Expand Down Expand Up @@ -65,6 +67,11 @@ const Grid = styled.div`
grid-template-rows: repeat(3, 100px);
`;

const logos = {
light: AGPLLogoLight,
dark: AGPLLogoDark,
};

export const LongContent = () => {
const theme = useTheme();
return (
Expand Down Expand Up @@ -92,7 +99,7 @@ export const LongContent = () => {
</P>
<P>
<div style={{ float: 'left', margin: '0 10px 10px 0' }}>
<img src={logos.oss[theme.type]} style={{ float: 'left' }} />
<img src={logos[theme.type]} style={{ float: 'left' }} />
</div>
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod
tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
Expand Down
gzdunek marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ export const ToolTipInfo: React.FC<
kind = 'info',
}) => {
const theme = useTheme();
const [anchorEl, setAnchorEl] = useState();
const [anchorEl, setAnchorEl] = useState<Element>();
const open = Boolean(anchorEl);

function handlePopoverOpen(event) {
function handlePopoverOpen(event: React.MouseEvent) {
setAnchorEl(event.currentTarget);
}

function handlePopoverClose() {
setAnchorEl(null);
setAnchorEl(undefined);
}

const triggerOnHoverProps = {
Expand Down Expand Up @@ -121,8 +121,6 @@ const ToolTipIcon = ({
return <WarningIcon $muteIconColor={muteIconColor} size="medium" />;
case 'error':
return <ErrorIcon $muteIconColor={muteIconColor} size="medium" />;
default:
kind satisfies never;
}
};

Expand Down
20 changes: 20 additions & 0 deletions web/packages/design/src/ToolTip/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Teleport
* Copyright (C) 2023 Gravitational, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

export { ToolTipInfo } from './ToolTip';
export { HoverTooltip } from './HoverTooltip';
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
import React from 'react';
import { Flex, LabelInput, Text } from 'design';

import { ToolTipInfo } from 'design/ToolTip';

import Select, { Option } from 'shared/components/Select';
import { ToolTipInfo } from 'shared/components/ToolTip';

export function AccessDurationRequest({
maxDuration,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import React from 'react';
import { Flex, Text } from 'design';

import { ToolTipInfo } from 'shared/components/ToolTip';
import { ToolTipInfo } from 'design/ToolTip';

import { AccessRequest } from 'shared/services/accessRequests';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ import React, { useState } from 'react';
import { Flex, Text, ButtonIcon, Box, LabelInput } from 'design';
import * as Icon from 'design/Icon';

import { ToolTipInfo } from 'design/ToolTip';

import Select, { Option } from 'shared/components/Select';
import { ToolTipInfo } from 'shared/components/ToolTip';

import { AccessRequest } from 'shared/services/accessRequests';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ import { ArrowBack, ChevronDown, ChevronRight, Warning } from 'design/Icon';
import Table, { Cell } from 'design/DataTable';
import { Danger } from 'design/Alert';

import { HoverTooltip } from 'design/ToolTip';

import Validation, { useRule, Validator } from 'shared/components/Validation';
import { Attempt } from 'shared/hooks/useAttemptNext';
import { pluralize } from 'shared/utils/text';
Expand All @@ -47,7 +49,6 @@ import { FieldCheckbox } from 'shared/components/FieldCheckbox';
import { mergeRefs } from 'shared/libs/mergeRefs';
import { TextSelectCopyMulti } from 'shared/components/TextSelectCopy';
import { RequestableResourceKind } from 'shared/components/AccessRequests/NewRequest/resource';
import { HoverTooltip } from 'shared/components/ToolTip';

import { CreateRequest } from '../../Shared/types';
import { AssumeStartTime } from '../../AssumeStartTime/AssumeStartTime';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,12 @@ import { ClickableLabelCell, Cell } from 'design/DataTable';

import { App } from 'teleport/services/apps';

import { ToolTipInfo } from 'design/ToolTip';

import Select, {
Option as BaseOption,
CustomSelectComponentProps,
} from 'shared/components/Select';
import { ToolTipInfo } from 'shared/components/ToolTip';

import { ResourceMap, RequestableResourceKind } from '../resource';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ import { ButtonPrimary, Text, Box, Alert, Flex, Label, H3 } from 'design';
import { Warning } from 'design/Icon';
import { Radio } from 'design/RadioGroup';

import { HoverTooltip } from 'design/ToolTip';

import Validation, { Validator } from 'shared/components/Validation';
import { FieldSelect } from 'shared/components/FieldSelect';
import { Option } from 'shared/components/Select';
import { Attempt } from 'shared/hooks/useAsync';
import { requiredField } from 'shared/components/Validation/rules';
import { HoverTooltip } from 'shared/components/ToolTip';
import { FieldTextArea } from 'shared/components/FieldTextArea';

import { AccessRequest, RequestState } from 'shared/services/accessRequests';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ import { displayDateWithPrefixedTime } from 'design/datetime';

import { LabelKind } from 'design/LabelState/LabelState';

import { HoverTooltip } from 'shared/components/ToolTip';
import { HoverTooltip } from 'design/ToolTip';

import { hasFinished, Attempt } from 'shared/hooks/useAsync';

import {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import { ButtonPrimary, Text, Box, ButtonIcon, Menu } from 'design';
import { Info } from 'design/Icon';
import { displayDateWithPrefixedTime } from 'design/datetime';

import { HoverTooltip } from 'shared/components/ToolTip';
import { HoverTooltip } from 'design/ToolTip';

import { AccessRequest } from 'shared/services/accessRequests';

export function PromotedMessage({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { Text, Toggle, Link, Flex, H2 } from 'design';

import { P } from 'design/Text/Text';

import { ToolTipInfo } from 'shared/components/ToolTip';
import { ToolTipInfo } from 'design/ToolTip';

const GUIDE_URL =
'https://goteleport.com/docs/reference/predicate-language/#resource-filtering';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { ChevronDown } from 'design/Icon';
import cfg from 'teleport/config';
import { Cluster } from 'teleport/services/clusters';

import { HoverTooltip } from 'shared/components/ToolTip';
import { HoverTooltip } from 'design/ToolTip';

export interface ClusterDropdownProps {
clusterLoader: ClusterLoader;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
import { ChevronDown } from 'design/Icon';
import { CheckboxInput } from 'design/Checkbox';

import { HoverTooltip } from 'shared/components/ToolTip';
import { HoverTooltip } from 'design/ToolTip';

type MultiselectMenuProps<T> = {
options: {
Expand Down
2 changes: 1 addition & 1 deletion web/packages/shared/components/Controls/SortMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import React, { useState } from 'react';
import { ButtonBorder, Flex, Menu, MenuItem } from 'design';
import { ArrowDown, ArrowUp } from 'design/Icon';

import { HoverTooltip } from 'shared/components/ToolTip';
import { HoverTooltip } from 'design/ToolTip';

type SortMenuSort<T extends object> = {
fieldName: Exclude<keyof T, symbol | number>;
Expand Down
2 changes: 1 addition & 1 deletion web/packages/shared/components/Controls/ViewModeSwitch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { Rows, SquaresFour } from 'design/Icon';

import { ViewMode } from 'gen-proto-ts/teleport/userpreferences/v1/unified_resource_preferences_pb';

import { HoverTooltip } from 'shared/components/ToolTip';
import { HoverTooltip } from 'design/ToolTip';

export const ViewModeSwitch = ({
currentViewMode,
Expand Down
3 changes: 2 additions & 1 deletion web/packages/shared/components/FieldInput/FieldInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ import styled, { useTheme } from 'styled-components';
import { IconProps } from 'design/Icon/Icon';
import { InputMode, InputSize, InputType } from 'design/Input';

import { ToolTipInfo } from 'design/ToolTip';

import { useRule } from 'shared/components/Validation';
import { ToolTipInfo } from 'shared/components/ToolTip';

const FieldInput = forwardRef<HTMLInputElement, FieldInputProps>(
(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ import React, { useState } from 'react';

import Box from 'design/Box';

import { Button } from 'design/Button';

import Validation from 'shared/components/Validation';

import { arrayOf, requiredField } from '../Validation/rules';

import { FieldMultiInput } from './FieldMultiInput';

export default {
Expand All @@ -30,7 +36,21 @@ export function Story() {
const [items, setItems] = useState([]);
return (
<Box width="500px">
<FieldMultiInput label="Some items" value={items} onChange={setItems} />
<Validation>
{({ validator }) => (
<>
<FieldMultiInput
label="Some items"
value={items}
onChange={setItems}
rule={arrayOf(requiredField('required'))}
/>
<Button mt={3} onClick={() => validator.validate()}>
Validate
</Button>
</>
)}
</Validation>
</Box>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,36 @@
import userEvent from '@testing-library/user-event';
import React, { useState } from 'react';

import { render, screen } from 'design/utils/testing';
import { act, render, screen } from 'design/utils/testing';

import Validation, { Validator } from 'shared/components/Validation';

import { arrayOf, requiredField } from '../Validation/rules';

import { FieldMultiInput, FieldMultiInputProps } from './FieldMultiInput';

const TestFieldMultiInput = ({
onChange,
refValidator,
...rest
}: Partial<FieldMultiInputProps>) => {
}: Partial<FieldMultiInputProps> & {
refValidator?: (v: Validator) => void;
}) => {
const [items, setItems] = useState<string[]>([]);
const handleChange = (it: string[]) => {
setItems(it);
onChange?.(it);
};
return <FieldMultiInput value={items} onChange={handleChange} {...rest} />;
return (
<Validation>
{({ validator }) => {
refValidator?.(validator);
return (
<FieldMultiInput value={items} onChange={handleChange} {...rest} />
);
}}
</Validation>
);
};

test('adding, editing, and removing items', async () => {
Expand Down Expand Up @@ -69,3 +85,35 @@ test('keyboard handling', async () => {
await user.keyboard('{Enter}bananas');
expect(onChange).toHaveBeenLastCalledWith(['apples', 'bananas', 'oranges']);
});

test('validation', async () => {
const user = userEvent.setup();
let validator: Validator;
render(
<TestFieldMultiInput
refValidator={v => {
validator = v;
}}
rule={arrayOf(requiredField('required'))}
/>
);

act(() => validator.validate());
expect(validator.state.valid).toBe(true);
expect(screen.getByRole('textbox')).toHaveAccessibleDescription('');

await user.click(screen.getByRole('button', { name: 'Add More' }));
await user.type(screen.getAllByRole('textbox')[1], 'foo');
act(() => validator.validate());
expect(validator.state.valid).toBe(false);
expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription(
'required'
);
expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription('');

await user.type(screen.getAllByRole('textbox')[0], 'foo');
act(() => validator.validate());
expect(validator.state.valid).toBe(true);
expect(screen.getAllByRole('textbox')[0]).toHaveAccessibleDescription('');
expect(screen.getAllByRole('textbox')[1]).toHaveAccessibleDescription('');
});
Loading
Loading