Skip to content

Add filter to cluster dropdown#37104

Merged
avatus merged 1 commit intomasterfrom
avatus/fix_cluster_dropdown
Jan 24, 2024
Merged

Add filter to cluster dropdown#37104
avatus merged 1 commit intomasterfrom
avatus/fix_cluster_dropdown

Conversation

@avatus
Copy link
Copy Markdown
Contributor

@avatus avatus commented Jan 23, 2024

Fixes #37014

Screenshot 2024-01-23 at 12 13 38 PM

This PR adds a search field filter to the cluster dropdown. Some choices we made with design

  • only show the input field if more than 5 clusters exist (when the menu starts to scroll)
  • remove the border/focus styles border on the input.. because it was ugly with it
  • bold the currently selected cluster in the dropdown
  • show "half" an item in the dropdown menu to suggest we can scroll down

If you want to test it locally, update the loadClusters function to something like this

        // const res = await loadClusters(signal.signal);
      const clusters = new Array(500).fill(null).map(
        (_, i) =>
          ({
            clusterId: `cluster-${i}`,
          } as Cluster)
      );
      setOptions(createOptions(clusters));
      return clusters;

@avatus avatus added backport-required no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels Jan 23, 2024
@avatus avatus requested review from kimlisa and rudream January 23, 2024 18:28
@rosstimothy
Copy link
Copy Markdown
Contributor

Does this also prevent the cluster dropdown from extending beyond the bottom of the page? If you look real hard in the screenshot that I attached in #37014 I was scrolled all the way to the end of the list and was unable to see the last entry.

@avatus
Copy link
Copy Markdown
Contributor Author

avatus commented Jan 23, 2024

Does this also prevent the cluster dropdown from extending beyond the bottom of the page?

Yes, the screenshot above has 500 entries in it. it will not grow larger than the screenshot height and can scroll to the bottom of the list

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'd write a story for this shared component and write states for empty, 1, couple, and a few hundred.

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.

matches the gap with the other filter dropdown

Suggested change
top: 240px !important;
top: 236px !important;

Comment on lines 167 to 170
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.

just as an fyi when you come back to tackle this, it still gets pushed up when a error banner renders

Comment on lines 200 to 205
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 think we can take this out so it isn't calculating when there is no filter?

let filteredOptions = options
if (clusterFilter) {
  filteredOptions = options
              .filter(cluster =>
                cluster.label
                  .toLowerCase()
                  .includes(clusterFilter.toLowerCase())
              )
}

Copy link
Copy Markdown
Contributor

@rudream rudream left a comment

Choose a reason for hiding this comment

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

LGTM once @kimlisa's comments are addressed

@avatus avatus force-pushed the avatus/fix_cluster_dropdown branch from bba2118 to 13ff0c1 Compare January 24, 2024 22:47
@avatus
Copy link
Copy Markdown
Contributor Author

avatus commented Jan 24, 2024

Force pushed because my branch got out of whack. added a story and fixed the padding

@rudream rudream self-requested a review January 24, 2024 23:13
@avatus avatus added this pull request to the merge queue Jan 24, 2024
Merged via the queue into master with commit 3c93c2d Jan 24, 2024
@avatus avatus deleted the avatus/fix_cluster_dropdown branch January 24, 2024 23:39
@public-teleport-github-review-bot
Copy link
Copy Markdown

@avatus See the table below for backport results.

Branch Result
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-required no-changelog Indicates that a PR does not require a changelog entry size/sm ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to see all trusted clusters

4 participants