Prepare `SearchBar` for the integration with unified resources by gzdunek · Pull Request #34402 · gravitational/teleport

gzdunek · 2023-11-09T17:35:26Z

I tried to explain a reason for each change in the commit message, let me know if something is unclear.
I'm going to backport this PR to v14 and v13 to make future backports easier. The change that allows selecting multiple resource filters probably won't be very useful without unified resources, but I think it is okay to backport it.

The most "controversial" may be 596e1e82ce6e0824dff34dec7ec08875c33221c6 that changes the way of rendering the search bar. It was based on a comment from Kenny, he suggested that we may wan't to switch to more "modal" version of the search bar, that is not restricted by the input width.
The biggest difference it makes when the window is narrow and the cluster selector is visible.

Before:

After:

IMO the controversial part is that the selector now opens horizontally centered, while the search input isn't centered. But I think that is fine, the command bar in VS code behaves very similarly.

Changelog: Allow selecting multiple resource filters in the search bar in Connect

This matches the latest design.

This will be needed for unified resources integration, where it is possible to select more than one filter.

Otherwise, we will have to convert the resource filter type to the type expected by the unified resources component and vice-versa. It is just easier to use the same type.

gzdunek · 2023-11-09T17:37:11Z

  const [isOpen, setIsOpen] = useState(false);
  const [inputValue, setInputValue] = useState('');
  const [activePicker, setActivePicker] = useState(actionPicker);
-  // TODO(ravicious): Consider using another data structure for search filters as we know that we


I removed the comment because now we can select multiple resource filters, and I think the current structure works quite well.

We could have an object with fields clusterFilter and resourceFilters, this way you wouldn't need to do the filtering based on the type field. From what I see, 90% of the type we end up either plucking the cluster filter from the array or filtering it down just resource filters.

There rest are places which have logic like this:

teleport/web/packages/teleterm/src/ui/Search/useSearch.ts

Lines 369 to 371 in 97c3757

// The preview should be fetched only when at least one filter is selected. Otherwise we'd send

// three requests for each connected cluster when the search bar gets open.

if (filters.length >= 1) {

It could be encapsulated in functions which work on the SearchFilters type (which would be that object with the fields I mentioned), here we could have something like hasActiveSearchFilters(filters: SearchFilters): boolean.

…aaand while writing this comment I realized what was the original requirement behind using an array – we need to keep the order of them so that if you first enter a resource filter and then a cluster filter, they're rendered in the UI in the same way.

Let's keep them this way I guess.

Yeah, the requirement to keep the filters in order didn't allow me to change the structure.

github-actions · 2023-11-09T17:39:29Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

ravicious

Looking good. 👍

ravicious · 2023-11-10T13:28:54Z

  const [isOpen, setIsOpen] = useState(false);
  const [inputValue, setInputValue] = useState('');
  const [activePicker, setActivePicker] = useState(actionPicker);
-  // TODO(ravicious): Consider using another data structure for search filters as we know that we


We could have an object with fields clusterFilter and resourceFilters, this way you wouldn't need to do the filtering based on the type field. From what I see, 90% of the type we end up either plucking the cluster filter from the array or filtering it down just resource filters.

There rest are places which have logic like this:

teleport/web/packages/teleterm/src/ui/Search/useSearch.ts

Lines 369 to 371 in 97c3757

// The preview should be fetched only when at least one filter is selected. Otherwise we'd send

// three requests for each connected cluster when the search bar gets open.

if (filters.length >= 1) {

It could be encapsulated in functions which work on the SearchFilters type (which would be that object with the fields I mentioned), here we could have something like hasActiveSearchFilters(filters: SearchFilters): boolean.

…aaand while writing this comment I realized what was the original requirement behind using an array – we need to keep the order of them so that if you first enter a resource filter and then a cluster filter, they're rendered in the UI in the same way.

Let's keep them this way I guess.

ravicious · 2023-11-10T13:31:19Z

-    // TODO(ravicious): Accept just `server | database | kube` as searchFilter here, wrap it in a
-    // variant of a discriminated union in searchResult.ts.
-    filter: ResourceTypeSearchFilter | undefined;
+    filters: SupportedResourceType[] | undefined;


The filter field used to be of type ResourceTypeSearchFilter | undefined because we wanted to communicate that there might be no filter. In cases where an array is accepted, I think it's better to require the callsite to always provide an array and represent the "no filter" case by passing an empty array.

ravicious · 2023-11-10T13:34:04Z

 };

-export type SupportedResourceType = 'kubes' | 'servers' | 'databases';
+export type SupportedResourceType = 'kube_cluster' | 'node' | 'db';


Could we use the Pick utility type to document that it's a subset of the type from unified resources?

That's a good idea, btw I had to use Extract utility type to make a subset of union (Pick doesn't work).

ravicious · 2023-11-10T13:35:00Z

+          'node' as const,
+          'db' as const,
+          'kube_cluster' as const,


const is not needed AFAIR if you declared the type of the variable itself.

They were needed here because the type was specified for an array that was then filtered and TS couldn't narrow down the type:

Type string is not assignable to type SupportedResourceType

I overcome this by extracting a const var:

const SUPPORTED_RESOURCE_TYPES: SupportedResourceType[] = [ 'node', 'db', 'kube_cluster', ];

ravicious · 2023-11-10T13:40:13Z

@@ -126,7 +121,7 @@ export function ActionPicker(props: { input: ReactElement }) {
      return (
        <FilterButton
          key="resource-type"


Keys need to be made unique, otherwise there's an error about them being the same and removing them is buggy.

Right, I forgot about this place.

ravicious · 2023-11-10T13:43:46Z

      <span
+        title={props.text}
        css={`
          max-width: calc(${props => props.theme.space[9]}px * 2);


I think we can remove max-width from here, with unified resources and the feedback we got about the connection picker we know how users feel about identifiers being cut off.

Without max-width, very long cluster names will be "naturally" cut off.

Done, I also removed text-overflow and overflow, they don't have any effect without max-width.

github-actions · 2023-11-13T10:42:41Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

public-teleport-github-review-bot · 2023-11-13T11:07:32Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v13	Create PR
branch/v14	Create PR

gzdunek added 4 commits November 9, 2023 18:21

Change styling of the filter buttons, add a close icon

ef99255

This matches the latest design.

Allow selecting multiple resource filters

ff869db

This will be needed for unified resources integration, where it is possible to select more than one filter.

Use the same filter types as unified resources

b9a767d

Otherwise, we will have to convert the resource filter type to the type expected by the unified resources component and vice-versa. It is just easier to use the same type.

Center picker container horizontally, don't allow it to be too narrow

34d0a47

gzdunek commented Nov 9, 2023

View reviewed changes

gzdunek requested review from avatus and ravicious November 9, 2023 17:38

gzdunek marked this pull request as ready for review November 9, 2023 17:38

gzdunek added backport/branch/v13 labels Nov 9, 2023

github-actions Bot added size/sm ui labels Nov 9, 2023

github-actions Bot requested review from rudream and ryanclark November 9, 2023 17:39

gzdunek removed request for rudream and ryanclark November 9, 2023 17:39

ravicious approved these changes Nov 10, 2023

View reviewed changes

avatus approved these changes Nov 10, 2023

View reviewed changes

gzdunek added 6 commits November 13, 2023 10:33

Remove undefined from searchResources.filters

1628eca

Move SUPPORTED_RESOURCE_TYPES to const

721b121

Make resource filter keys unique

a80a678

Remove max width from the filter buttons

23f674a

Extract SupportedResourceType from the unified resource type

fb6fc91

Rename SupportedResourceType to ResourceTypeFilter

79dc462

gzdunek added this pull request to the merge queue Nov 13, 2023

Merged via the queue into master with commit 2d83292 Nov 13, 2023

gzdunek deleted the gzdunek/prepare-search-bar branch November 13, 2023 11:06

This was referenced Nov 13, 2023

[v14] Prepare SearchBar for the integration with unified resources #34543

Merged

[v13] Prepare SearchBar for the integration with unified resources #34544

Merged

Fixed resource type filters to be matched by a readable name #34777

Merged

	// The preview should be fetched only when at least one filter is selected. Otherwise we'd send
	// three requests for each connected cluster when the search bar gets open.
	if (filters.length >= 1) {

Conversation

gzdunek commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 9, 2023

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 13, 2023

Uh oh!

public-teleport-github-review-bot Bot commented Nov 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gzdunek commented Nov 9, 2023 •

edited

Loading