Skip to content

Unified Resources UI: Various Fixes #32064

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.

Sign up for GitHub

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

Merged
avatus merged 10 commits into from
Sep 19, 2023
Merged

Unified Resources UI: Various Fixes #32064

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f4cd2f5
Remove old indicator from unified resources
avatus Sep 15, 2023
f2b67bd
Use lowercase for unified name sort
avatus Sep 15, 2023
3512e4b
Include friendly name in app icon guess
avatus Sep 15, 2023
527a4b0
Add filters exist indicator
avatus Sep 16, 2023
c0c694e
Remove uppercase from filter buttons and add filter count
avatus Sep 18, 2023
cb2affb
Merge branch 'master' into avatus/unified_fixes
avatus Sep 18, 2023
6c33fcf
Remove indicator import
avatus Sep 18, 2023
642be8c
Resource custom sort by contained resource
avatus Sep 18, 2023
91cb348
Add unified name compare test
avatus Sep 18, 2023
a05331c
Merge branch 'master' into avatus/unified_fixes
avatus Sep 18, 2023
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
2 changes: 1 addition & 1 deletion api/types/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ func unifiedNameCompare(a ResourceWithLabels, b ResourceWithLabels, isDesc bool)
nameB = b.GetName()
}

return stringCompare(nameA, nameB, isDesc)
return stringCompare(strings.ToLower(nameA), strings.ToLower(nameB), isDesc)

@kimlisa kimlisa Sep 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add test?

@avatus avatus Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I believe I have the test testing what we want 91cb348

}

func (r ResourcesWithLabels) SortByCustom(by SortBy) error {
Expand Down
47 changes: 46 additions & 1 deletion web/packages/teleport/src/UnifiedResources/FilterPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

import React, { useState } from 'react';
import styled from 'styled-components';
import { ButtonBorder, ButtonPrimary, ButtonSecondary } from 'design/Button';
import { SortDir } from 'design/DataTable/types';
import { Text } from 'design';
Expand Down Expand Up @@ -151,6 +152,14 @@ const FilterTypesMenu = ({
setKinds(newKinds);
};

const handleSelectAll = () => {
setKinds(kindOptions.map(k => k.value));
};

const handleClearAll = () => {
setKinds([]);
};

Comment on lines +155 to +162

@avatus avatus Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These methods seem a bit simple atm, but when we start to add more filter types besides just kinds they will grow a bit.

const applyFilters = () => {
onChange(kinds);
handleClose();
Expand All @@ -167,8 +176,9 @@ const FilterTypesMenu = ({
size="small"
onClick={handleOpen}
>
Type
Types {kindsFromParams.length > 0 ? `(${kindsFromParams.length})` : ''}

@avatus avatus Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Similarly, this will change once we add more filters but abstracting now seemed a bit frivolous

<ChevronDown ml={2} size="small" color="text.slightlyMuted" />
{kindsFromParams.length > 0 && <FiltersExistIndicator />}
</ButtonSecondary>
<Menu
popoverCss={() => `margin-top: 36px;`}
Expand All @@ -184,6 +194,30 @@ const FilterTypesMenu = ({
open={Boolean(anchorEl)}
onClose={cancelUpdate}
>
<Flex gap={2} p={2}>
<ButtonSecondary
size="small"
onClick={handleSelectAll}
textTransform="none"
css={`
background-color: transparent;
`}
px={2}
>
Select All
</ButtonSecondary>
<ButtonSecondary
size="small"
onClick={handleClearAll}
textTransform="none"
css={`
background-color: transparent;
`}
px={2}
>
Clear All
</ButtonSecondary>
</Flex>
{kindOptions.map(kind => (
<MenuItem
px={2}
Expand Down Expand Up @@ -320,3 +354,14 @@ function kindArraysEqual(arr1: string[], arr2: string[]) {

return true;
}

const FiltersExistIndicator = styled.div`
position: absolute;
top: -4px;
right: -4px;
height: 12px;
width: 12px;
background-color: ${props => props.theme.colors.brand};
border-radius: 50%;
display: inline-block;
`;
10 changes: 0 additions & 10 deletions web/packages/teleport/src/UnifiedResources/Resources.tsx
View file
Open in desktop

@kimlisa kimlisa Sep 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i wanted to test this change out in storybook, but i think most of it got broken along the way. doesn't seem like this PR would've broke it, or am i missing something? (i saw these stories in working state like couple weeks ago)

@avatus avatus Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Storybook is working for me. What errors are you seeing?

@kimlisa kimlisa Sep 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

weird, i tried it again (404)
image

@avatus avatus Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, I believe that is related to changes made here #30964

@bl-nero bl-nero Sep 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm... Folks, I don't want to sound cliche, but it works for me. :D

@bl-nero bl-nero Sep 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holy cow, I think I know what the issue is. I just noticed that you are using your machine's explicit IP address, and I was always using 127.0.0.1 instead. If I substitute my machine's IP, it ends up on 404, too. It looks like that mock service worker Storybook plugin fails to load resources with a different IP. I'll take a look and see if it can be configured another way, since I'd really like to avoid moving away from it (it was the only way I could get both API calls and SVG loading to work). In the meantime, please use 127.0.0.1.

@bl-nero bl-nero Sep 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really know how to fix it. :( It might be related to security restrictions (apparently, service workers are not allowed for pages not running on HTTPS if they're not accessed using localhost URLs). The mystery is why then other tests work, so I'm more inclined to believe that this is plugin-specific (other tests use MSW directly). I have a more important thing to do at the moment (backporting that PR mentioned above), but perhaps I'll give it one more try in the afternoon. Perhaps upgrading Storybook will change something; the version we're using is a bit old and ratchety.

Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,6 @@ export function Resources() {
</ResourcesContainer>
<div ref={setScrollDetector} />
<ListFooter>
<IndicatorContainer status={attempt.status}>
<Indicator size={INDICATOR_SIZE} />
</IndicatorContainer>
{attempt.status === 'failed' && resources.length > 0 && (
<ButtonSecondary onClick={onRetryClicked}>Load more</ButtonSecondary>
)}
Expand Down Expand Up @@ -253,13 +250,6 @@ const ListFooter = styled.div`
text-align: center;
`;

// Line height is set to 0 to prevent the layout engine from adding extra pixels
// to the element's height.
const IndicatorContainer = styled(Box)`
display: ${props => (props.status === 'processing' ? 'block' : 'none')};
line-height: 0;
`;

const emptyStateInfo: EmptyStateInfo = {
title: 'Add your first resource to Teleport',
byline:
Expand Down
5 changes: 4 additions & 1 deletion web/packages/teleport/src/services/apps/makeApps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,28 +84,31 @@ export default function makeApp(json: any): App {
}

function guessAppIcon(json: any): GuessedAppType {
const { name, labels, awsConsole = false } = json;
const { name, labels, friendlyName, awsConsole = false } = json;

if (awsConsole) {
return 'Aws';
}

if (
name?.toLocaleLowerCase().includes('slack') ||
friendlyName?.toLocaleLowerCase().includes('slack') ||
labels?.some(l => `${l.name}:${l.value}` === 'icon:slack')
) {
return 'Slack';
}

if (
name?.toLocaleLowerCase().includes('grafana') ||
friendlyName?.toLocaleLowerCase().includes('grafana') ||
labels?.some(l => `${l.name}:${l.value}` === 'icon:grafana')
) {
return 'Grafana';
}

if (
name?.toLocaleLowerCase().includes('jenkins') ||
friendlyName?.toLocaleLowerCase().includes('jenkins') ||
labels?.some(l => `${l.name}:${l.value}` === 'icon:jenkins')
) {
return 'Jenkins';
Expand Down