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

Fix UI issues after introducing new button design system #43663

Merged
merged 14 commits into from
Jul 10, 2024
Merged
854 changes: 436 additions & 418 deletions web/packages/design/src/Button/__snapshots__/buttons.story.test.tsx.snap

Large diffs are not rendered by default.

61 changes: 34 additions & 27 deletions web/packages/design/src/Button/buttons.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export default {
export const Buttons = () => {
const fills: ButtonFill[] = ['filled', 'minimal', 'border'];
return (
<Flex gap={4} flexDirection="column" alignItems="flex-start">
<Flex gap={5} flexDirection="column" alignItems="flex-start">
<Table>
<thead>
<tr>
Expand Down Expand Up @@ -100,19 +100,24 @@ export const Buttons = () => {
<Button size="medium">Medium</Button>
<Button size="small">Small</Button>
</Flex>
<Input defaultValue="Some text" />
<Button size="extra-large" inputAlignment>
Extra large with input alignment
</Button>
<Button size="large" inputAlignment>
Large with input alignment
</Button>
<Button size="medium" inputAlignment>
Medium with input alignment
</Button>
<Button size="small" inputAlignment>
Small with input alignment
</Button>
<Flex flexDirection="column" gap={3} alignItems="flex-start">
<Input
defaultValue="Padding of buttons below should match padding of this input"
width="480px"
/>
<Button size="extra-large" inputAlignment>
Extra large with input alignment
</Button>
<Button size="large" inputAlignment>
Large with input alignment
</Button>
<Button size="medium" inputAlignment>
Medium with input alignment
</Button>
<Button size="small" inputAlignment>
Small with input alignment
</Button>
</Flex>
<Button block>block = true</Button>
<Flex gap={3}>
<Button disabled>Disabled</Button>
Expand Down Expand Up @@ -167,19 +172,21 @@ export const Buttons = () => {
<ButtonLink href="">Button Link</ButtonLink>
<ButtonText>Button Text</ButtonText>
</Flex>
{[2, 1, 0].map(size => (
<Flex gap={3} key={`size-${size}`}>
<ButtonIcon size={size}>
<icons.AddUsers />
</ButtonIcon>
<ButtonIcon size={size}>
<icons.Ellipsis />
</ButtonIcon>
<ButtonIcon size={size} disabled>
<icons.Trash />
</ButtonIcon>
</Flex>
))}
<Flex gap={3} flexDirection="column" alignItems="flex-start">
{[2, 1, 0].map(size => (
<Flex gap={3} key={`size-${size}`}>
<ButtonIcon size={size}>
<icons.AddUsers />
</ButtonIcon>
<ButtonIcon size={size}>
<icons.Ellipsis />
</ButtonIcon>
<ButtonIcon size={size} disabled>
<icons.Trash />
</ButtonIcon>
</Flex>
))}
</Flex>
</Flex>
);
};
Expand Down
1 change: 1 addition & 0 deletions web/packages/design/src/ButtonLink/ButtonLink.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const StyledButtonLink = styled.a`
&:hover,
&:focus {
color: ${({ theme }) => theme.colors.buttons.link.hover};
box-shadow: none;
Copy link
Member

Choose a reason for hiding this comment

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

I think it shouldn't have a background on hover either. The way I think about it is that ButtonLink is supposed to behave just like Link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're probably right. I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not going to change it right now, because it breaks a bit in places where link buttons are placed on a red background (the text turns illegible). As this coincides with your another comment, I'll fix it properly after I deal with the alert banners.

}

&:active {
Expand Down
2 changes: 1 addition & 1 deletion web/packages/shared/components/ButtonSso/ButtonSso.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const ButtonSso = forwardRef<HTMLInputElement, Props>((props: Props, ref) => {
const { color, Icon } = getSSOIcon(ssoType);

return (
<StyledButton color={color} block {...rest} ref={ref}>
<StyledButton size="extra-large" color={color} block {...rest} ref={ref}>
{Boolean(Icon) && (
<IconBox>
<Icon data-testid="icon" color="white" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,7 @@ export const ButtonTextWithAddIcon = ({
iconSize?: number | 'small' | 'medium' | 'large' | 'extraLarge';
}) => {
return (
<ButtonText
onClick={onClick}
css={`
padding-left: 0px;
&:disabled {
.icon-add {
opacity: 0.35;
}
pointer-events: none;
}
`}
disabled={disabled}
>
<ButtonText onClick={onClick} inputAlignment disabled={disabled}>
<AddIcon
className="icon-add"
size={iconSize}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,7 @@ export function ClusterDropdown({
return (
<Flex textAlign="center" alignItems="center" mb={mb}>
<HoverTooltip tipContent={'Select cluster'}>
<ButtonSecondary
px={2}
css={`
border-color: ${props => props.theme.colors.spotBackground[0]};
`}
textTransform="none"
size="small"
onClick={handleOpen}
>
<ButtonSecondary size="small" onClick={handleOpen}>
Cluster: {selectedOption.label}
<ChevronDown ml={2} size="small" color="text.slightlyMuted" />
</ButtonSecondary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/**
* Teleport
* Copyright (C) 2024 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/>.
*/

import React from 'react';

import ConsoleContextProvider from 'teleport/Console/consoleContextProvider';
import ConsoleContext from 'teleport/Console/consoleContext';
import { FileTransferRequest } from 'teleport/Console/DocumentSsh/useFileTransfer';

import { FileTransferRequests } from './';

export default {
title: 'Shared/FileTransfer',
};

export const Requests = () => {
const conCtx = new ConsoleContext();
conCtx.storeUser.setState({ username: 'bob' });
return (
<ConsoleContextProvider value={conCtx}>
<FileTransferRequests
requests={requests}
onApprove={() => {}}
onDeny={() => {}}
/>
</ConsoleContextProvider>
);
};

const requests: FileTransferRequest[] = [
{
sid: 'dummy-sid',
requestID: 'dummy-request-id',
requester: 'alice',
approvers: [],
location: '/etc/teleport.yaml',
download: true,
},
{
sid: 'dummy-sid',
requestID: 'dummy-request-id',
requester: 'john',
approvers: ['bob'],
location: '/home/alice/.ssh/config',
download: true,
},
];
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import React from 'react';
import styled from 'styled-components';
import { ButtonBorder, Box, Flex, Text } from 'design';
import { ButtonBorder, Box, Flex, Text, Button } from 'design';
import * as Icons from 'design/Icon';
import {
FileTransferRequest,
Expand Down Expand Up @@ -130,18 +130,25 @@ const ResponseForm = ({
{getPendingText(request)}
</Text>
<Flex gap={2}>
<ButtonBorder
<Button
fill="border"
intent="success"
disabled={request.approvers.includes(currentUser.username)}
block
onClick={() => onApprove(request.requestID, true)}
>
<Icons.Check size="small" mr={2} />
Approve
</ButtonBorder>
<ButtonBorder block onClick={() => onDeny(request.requestID, false)}>
</Button>
<Button
fill="border"
intent="danger"
block
onClick={() => onDeny(request.requestID, false)}
>
<Icons.Cross size="small" mr={2} />
Deny
</ButtonBorder>
</Button>
</Flex>
</Box>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,13 @@ export default class MenuActionIcon extends React.Component<
return (
<>
<ButtonBorder
height="24px"
size="small"
setRef={e => (this.anchorEl = e)}
onClick={this.onOpen}
{...buttonProps}
>
{this.props.buttonText || 'OPTIONS'}
<ChevronDown ml={2} mr={-2} size="small" color="text.slightlyMuted" />
{this.props.buttonText || 'Options'}
Comment on lines 61 to +67
Copy link
Member

Choose a reason for hiding this comment

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

Just my opinion, but it's quite distracting for me how <Button fill="border"> loses its border on hover. 😅 But I suspect it was hard to represent the difference between all five states without this.

<ChevronDown ml={2} size="small" color="text.slightlyMuted" />
</ButtonBorder>
<Menu
getContentAnchorEl={null}
Expand Down
3 changes: 1 addition & 2 deletions web/packages/shared/components/MenuLogin/MenuLogin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,12 @@ export const MenuLogin = React.forwardRef<MenuLoginHandle, MenuLoginProps>(
<ButtonBorder
width={alignButtonWidthToMenu ? width : null}
textTransform={props.textTransform}
height="24px"
size="small"
setRef={anchorRef}
onClick={onOpen}
>
Connect
<ChevronDown ml={1} mr={-2} size="small" color="text.slightlyMuted" />
<ChevronDown ml={1} size="small" color="text.slightlyMuted" />
</ButtonBorder>
<Menu
anchorOrigin={anchorOrigin}
Expand Down
11 changes: 1 addition & 10 deletions web/packages/shared/components/UnifiedResources/FilterPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -267,15 +267,7 @@ const FilterTypesMenu = ({
return (
<Flex textAlign="center" alignItems="center">
<HoverTooltip tipContent={'Filter by resource type'}>
<ButtonSecondary
px={2}
css={`
border-color: ${props => props.theme.colors.spotBackground[0]};
`}
textTransform="none"
size="small"
onClick={handleOpen}
>
<ButtonSecondary size="small" onClick={handleOpen}>
Types{' '}
{kindsFromParams.length > 0 ? `(${kindsFromParams.length})` : ''}
<ChevronDown ml={2} size="small" color="text.slightlyMuted" />
Expand Down Expand Up @@ -446,7 +438,6 @@ const SortMenu: React.FC<SortMenuProps> = props => {
onClick={onDirChange}
textTransform="none"
css={`
width: 0px; // remove extra width around the button icon
border-top-left-radius: 0;
border-bottom-left-radius: 0;
border-color: ${props => props.theme.colors.spotBackground[2]};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,7 @@ import React, {
} from 'react';

import styled from 'styled-components';
import {
Box,
Flex,
ButtonLink,
ButtonSecondary,
Text,
ButtonBorder,
} from 'design';
import { Box, Flex, Button, ButtonSecondary, Text, ButtonBorder } from 'design';
import { Icon, Magnifier, PushPin } from 'design/Icon';
import { Danger } from 'design/Alert';

Expand Down Expand Up @@ -455,7 +448,9 @@ export function UnifiedResources(props: UnifiedResourcesProps) {
{resourcesFetchAttempt.statusCode !== 400 &&
resourcesFetchAttempt.statusCode !== 403 && (
<Box flex="0 0 auto" ml={2}>
<ButtonLink onClick={onRetryClicked}>Retry</ButtonLink>
<Button type="button" onClick={onRetryClicked}>
Retry
</Button>
Copy link
Member

Choose a reason for hiding this comment

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

This button didn't look quite right. I changed it a little bit, but it still doesn't look the best. idk what to do here, as we can't use fill any other than filled, because otherwise it blends with the red background. Also I wish the language server would suggest me "custom-defined" props first, because now if I place my cursor inside Button and ask my editor what props are available, it suggests me a bunch of aria props. ;f

@gzdunek Do you remember if there was a reason ButtonLink was used here instead of Button? This link doesn't have a href and we add onClick on it, which makes Button more suitable for it.

Before After
retry-before retry-after

Copy link
Contributor

Choose a reason for hiding this comment

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

@gzdunek Do you remember if there was a reason ButtonLink was used here instead of Button? This link doesn't have a href and we add onClick on it, which makes Button more suitable for it.

Hmm it wasn't me who added that button 😅 but I agree, Buttons fits better here.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just did a quick git blame and didn't dig further. 😏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave it as is, since it's exactly how it looks now, and one of the next steps on my roadmap is to deal with the error banners, which will include modernizing their action buttons: https://www.figma.com/design/Gpjs9vjhzUKF1GDbeG9JGE/Application-Design-System?node-id=8890-5504&m=dev

</Box>
)}
</Danger>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ function ConnectorListItem({ name, id, onEdit, onDelete }) {
</Text>
</Flex>
<ButtonPrimary mt="auto" size="medium" block onClick={onClickEdit}>
EDIT CONNECTOR
Edit Connector
</ButtonPrimary>
</ResponsiveConnector>
);
Expand Down
4 changes: 2 additions & 2 deletions web/packages/teleport/src/Bots/List/BotList.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ test('renders View options if type is github actions ssh', async () => {
const props = makeProps();
props.bots = [bot];
render(<BotList {...props} />);
fireEvent.click(await screen.findByText('OPTIONS'));
fireEvent.click(await screen.findByText('Options'));
expect(screen.getByText('View...')).toBeInTheDocument();
});

Expand All @@ -95,6 +95,6 @@ test('doesnt renders View options if bot type is not github actions', async () =
const props = makeProps();
props.bots = [bot];
render(<BotList {...props} />);
fireEvent.click(await screen.findByText('OPTIONS'));
fireEvent.click(await screen.findByText('Options'));
expect(screen.queryByText('View...')).not.toBeInTheDocument();
});
4 changes: 2 additions & 2 deletions web/packages/teleport/src/Bots/List/Bots.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ test('calls edit endpoint', async () => {
).toBeInTheDocument();
});

const actionCells = screen.queryAllByRole('button', { name: 'OPTIONS' });
const actionCells = screen.queryAllByRole('button', { name: 'Options' });
expect(actionCells).toHaveLength(botsApiResponseFixture.items.length);
await userEvent.click(actionCells[0]);

Expand Down Expand Up @@ -96,7 +96,7 @@ test('calls delete endpoint', async () => {
).toBeInTheDocument();
});

const actionCells = screen.queryAllByRole('button', { name: 'OPTIONS' });
const actionCells = screen.queryAllByRole('button', { name: 'Options' });
expect(actionCells).toHaveLength(botsApiResponseFixture.items.length);
await userEvent.click(actionCells[0]);

Expand Down
12 changes: 9 additions & 3 deletions web/packages/teleport/src/Bots/List/Bots.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import React, { useEffect, useState } from 'react';
import { useAttemptNext } from 'shared/hooks';
import { Link } from 'react-router-dom';
import { HoverTooltip } from 'shared/components/ToolTip';
import { Alert, Box, ButtonPrimary, Indicator } from 'design';
import { Alert, Box, Button, Indicator } from 'design';

import {
FeatureBox,
Expand Down Expand Up @@ -120,15 +120,21 @@ export function Bots() {
to request bot creation permissions.`
}
>
<ButtonPrimary
<Button
intent="primary"
fill={
fetchAttempt.status === 'success' && bots.length === 0
? 'filled'
: 'border'
}
ml="auto"
width="240px"
as={Link}
to={cfg.getBotsNewRoute()}
disabled={!hasAddBotPermissions}
>
Enroll New Bot
</ButtonPrimary>
</Button>
</HoverTooltip>
</Box>
</FeatureHeader>
Expand Down
Loading
Loading