Web: Add filters and pinning support for Enroll Resources page by kimlisa · Pull Request #51893 · gravitational/teleport

kimlisa · 2025-02-05T20:33:00Z

RFD that also links to figma

requires Wrap discover guide preference "pinned" field inside a proto message #52017
converted search bar to be type and enter like it is everywhere else
added filter dropdowns to filter by resource kind and hosting platform
added pinning support where users can pin any guides and it will be saved in user preference
- if user initially had no pin preferences, it'll render default pins

play with filter in story: https://localhost:9002/?path=/story/teleport-discover-selectresource--all-access

demo:

~~double checking if that's the intended behavior (where it resets to default if you remove all pins)~~

Screen.Recording.2025-02-05.at.12.29.37.PM.mov

changelog: Add filter drop-downs and pinning support for the "Enroll a New Resource" page in the web UI

TODO in another PR:

consolidate linux tiles into one, and add them as part of default pins
add URL query param support and remove the use of url loc state
add responsiveness (remove the 1000 min width requirement)

thedevelopnik · 2025-02-05T20:50:36Z

The expected behavior is that it should respect the user's choices and if they unpin everything not to re-pin things. The default pins are meant for new users coming in to the system to draw attention to good places to start. If they are not new users, know what they want, and have elected to remove those, I think putting them back would be frustrating.

zmb3 · 2025-02-05T20:57:05Z

Why does there seem to be a full-page refresh if you remove the last pin?

kimlisa · 2025-02-05T20:59:10Z

Why does there seem to be a full-page refresh if you remove the last pin?

@zmb3 it was manual refresh, to demonstrate that default pins came back on reload, after unpinning everything. but i'll be changing this behavior (which will be a backend change and will be in another PR)

gzdunek

First pass.

gzdunek

I will test it tomorrow.

gzdunek · 2025-02-11T15:54:32Z

-          <StyledInput
+      {updateDiscoverPreferenceAttempt.status === 'error' && (
+        <Danger mt={5} details={updateDiscoverPreferenceAttempt.statusText}>
+          Could not update pinned resources:


(the details are rendered in a separate line)

Suggested change

Could not update pinned resources:

Could not update pinned resources

i was following the unified resources design:

I believe it's a leftover from time when Alert couldn't display a title, so we needed a colon to separate it from the content. Now it can be removed.

gzdunek · 2025-02-11T16:06:19Z

      },
    } as UserPreferences;
+
+    if (setPreferenceAfterSuccess) {


Why do we need it? Is updating pins here different from updating them in unified resources?

it was to prevent any render change when fetch failed, without this flag the function updated local preference despite error which would make it appear to have been successful

I could define another function similarly done for unified resources. Let me know what you prefer, i thought the flag made it more obvious that behavior can be different

also the pinned resources works a bit differently for unified resources, it keeps pinned resources in a cluster preference object which is a ref and it calls its own function to update, but the end behavior is the same though which is to render updated pinned state only after success

I'm not sure if this is worth making updatePreferences logic more complex. This function is used in many places, and the current behavior has worked fine so far - maybe it can stay the same here? My main concern is that it might not be obvious when setPreferenceAfterSuccess should be set.

sure, i reverted the change and made a new function just for updating discover resource preference

gzdunek

With a few comments.

gzdunek · 2025-02-12T11:39:25Z

+          </Grid>
+        </Box>
+      )}
+      {filteredResources && filteredResources.length > 0 && (


Suggested change

{filteredResources && filteredResources.length > 0 && (

{filteredResources.length > 0 && (

gzdunek · 2025-02-12T11:40:59Z

+  onChangeShowApp(showApp: boolean): void;
+  onSelectResource(selectedResourceSpec: SelectResourceSpec): void;
+  pinningSupport: PinningSupport;
+  onChangePinGuide(guideId: string): void;


We don't use guide in other props.

Suggested change

onChangePinGuide(guideId: string): void;

onChangePin(guideId: string): void;

gzdunek · 2025-02-12T11:41:40Z

+                <Flex alignItems="center" gap={2}>
+                  <StyledText wantLargeTile={wantLargeTile}>{title}</StyledText>
+                  {resourceSpec.hasAccess && (
+                    <NewTabIcon color="text.muted" size={16} />


Let's use a predefined value ('small' in this case).

gzdunek · 2025-02-12T11:45:46Z

@@ -90,61 +111,125 @@ export function Tile({
  //     popup modal where it shows user to add app manually or automatically.
  return (
    <ResourceCard


We can add tabIndex={0} to make it navigable with keyboard.

gzdunek · 2025-02-12T12:39:14Z

      },
    } as UserPreferences;
+
+    if (setPreferenceAfterSuccess) {


I'm not sure if this is worth making updatePreferences logic more complex. This function is used in many places, and the current behavior has worked fine so far - maybe it can stay the same here? My main concern is that it might not be obvious when setPreferenceAfterSuccess should be set.

gzdunek · 2025-02-12T12:40:51Z

-          <StyledInput
+      {updateDiscoverPreferenceAttempt.status === 'error' && (
+        <Danger mt={5} details={updateDiscoverPreferenceAttempt.statusText}>
+          Could not update pinned resources:


I believe it's a leftover from time when Alert couldn't display a title, so we needed a colon to separate it from the content. Now it can be removed.

- Stop propagation and default behavior when clicking on pin button - Add small or large icon option for discover icons

public-teleport-github-review-bot · 2025-02-14T06:14:38Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v17	Failed

* Minor changes - Stop propagation and default behavior when clicking on pin button - Add small or large icon option for discover icons * Add pinning guide support * Fix lint

…tational#51893) * Minor changes - Stop propagation and default behavior when clicking on pin button - Add small or large icon option for discover icons * Add pinning guide support * Fix lint

…52176) * Web: make SelectResource.tsx component leaner (#51698) * Web: Move resource specs into resource directory * Web: move code out into separate files * Web: define discover resource guide ids and add discover user preference field (#51672) * Add discover resource preferences to user preferences * Define hard coded guide id consts * Set guide ids for resources * Add test * Address CRs * Define a new type just for SelectResource.tsx which requires id field * Wrap discover guide preference "pinned" field inside a proto message (#52017) This allows us to use "nil" value to mean no discover guide preferences set (which is used to set default values in the web UI). * Web: Add filters and pinning support for Enroll Resources page (#51893) * Minor changes - Stop propagation and default behavior when clicking on pin button - Add small or large icon option for discover icons * Add pinning guide support * Fix lint

kimlisa added the backport/branch/v17 label Feb 5, 2025

github-actions Bot requested review from avatus and gzdunek February 5, 2025 20:33

github-actions Bot added size/lg ui labels Feb 5, 2025

avatus reviewed Feb 6, 2025

View reviewed changes

kimlisa requested a review from avatus February 7, 2025 00:36

kimlisa force-pushed the lisa/ui-pin-guides branch from d049177 to 3761d0b Compare February 7, 2025 00:42

gzdunek reviewed Feb 7, 2025

View reviewed changes

kimlisa mentioned this pull request Feb 10, 2025

Wrap discover guide preference "pinned" field inside a proto message #52017

Merged

kimlisa force-pushed the lisa/ui-pin-guides branch 2 times, most recently from ab7f856 to 504abbf Compare February 11, 2025 09:22

kimlisa requested a review from gzdunek February 11, 2025 09:22

avatus approved these changes Feb 11, 2025

View reviewed changes

gzdunek reviewed Feb 11, 2025

View reviewed changes

kimlisa force-pushed the lisa/ui-pin-guides branch 2 times, most recently from e6833b3 to a30b299 Compare February 11, 2025 19:23

kimlisa requested a review from gzdunek February 11, 2025 19:23

gzdunek approved these changes Feb 12, 2025

View reviewed changes

kimlisa added 2 commits February 13, 2025 21:26

Minor changes

0aae97a

- Stop propagation and default behavior when clicking on pin button - Add small or large icon option for discover icons

Add pinning guide support

53fce72

kimlisa force-pushed the lisa/ui-pin-guides branch from f72ebb1 to 53fce72 Compare February 14, 2025 05:46

kimlisa enabled auto-merge February 14, 2025 05:50

Fix lint

0bd45e5

kimlisa added this pull request to the merge queue Feb 14, 2025

Merged via the queue into master with commit 5c859a8 Feb 14, 2025

kimlisa deleted the lisa/ui-pin-guides branch February 14, 2025 06:13

kimlisa mentioned this pull request Feb 14, 2025

[v17] Web: Add filters and pinning support for Enroll Resources page #52176

Merged

	Could not update pinned resources:
	Could not update pinned resources

	{filteredResources && filteredResources.length > 0 && (
	{filteredResources.length > 0 && (

	onChangePinGuide(guideId: string): void;
	onChangePin(guideId: string): void;

Conversation

kimlisa commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thedevelopnik commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmb3 commented Feb 5, 2025

Uh oh!

kimlisa commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gzdunek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gzdunek 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!

Uh oh!

Uh oh!

Uh oh!

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!

kimlisa Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek 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!

public-teleport-github-review-bot Bot commented Feb 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kimlisa commented Feb 5, 2025 •

edited

Loading

thedevelopnik commented Feb 5, 2025 •

edited

Loading

kimlisa commented Feb 5, 2025 •

edited

Loading

kimlisa Feb 14, 2025 •

edited

Loading