Web: Add plugin status types (okta) and add `error` kind to ToolTIp.tsx #44788

kimlisa · 2024-07-30T00:27:20Z

related: #44431

🛑 do not merge: backend work is required that is TBD, it may change some fields in this PR

this PR contains just the necessary changes to implement the okta status page located in enterprise

in the integrations list, only okta will have a view status link

added a error kind to tool tip

github-actions · 2024-07-30T00:27:49Z

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.

github-actions · 2024-07-30T00:29:19Z

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.

ryanclark · 2024-07-30T12:12:38Z

I feel like this option being hidden under "Options" would mean no one would know its there - can the row be clickable, or can we display "View Status" as a button and have a delete icon button too?

cc @roraback

gzdunek · 2024-07-30T15:57:33Z

Also a UX question: should we put error messages in tooltips?
Maybe we should just always show them on the page?

When there are sync errors (screesnohst), there are three warning icons. To see the actual problem, I have to click/hover them one by one (it feels inconvenient).

roraback · 2024-07-31T15:19:28Z

I feel like this option being hidden under "Options" would mean no one would know its there - can the row be clickable, or can we display "View Status" as a button and have a delete icon button too?

I agree. A few thoughts on this:

As we start to build out detail pages, I'd love for the entire row (or card) to become clickable so that users can navigate more easily.
I think we should also leave the option in the dropdown, especially since other integrations (and other tables) won't be clickable, so users may go to the dropdown before realizing that the entire row is clickable.
I think the View Status option should be above the delete option in the drop-down menu. Destructive actions should typically be the last option in a menu.
No ellipsis after the "View Status" text in the menu. Ellipses in a menu typically indicate that a follow-up action or input is required to complete the action, but there is no required follow-up to view the status. You just go to the status page.

roraback · 2024-07-31T15:28:19Z

Also a UX question: should we put error messages in tooltips?
Maybe we should just always show them on the page?

When there are sync errors, there are three warning icons. To see the actual problem, I have to click/hover them one by one (it feels inconvenient).

I hear you here, but trying to show errors inline can make the page look quite messy if we don't know what length we need to plan for. That said, I think it might be good to show some error text along the lines of "Failed 3m ago" alongside the warning icon, and make the whole thing a tooltip target so you have a bigger area to hover than just a little square icon.

What do you think about that @gzdunek @kimlisa ? It still requires hovering for the details, but it gives you a bit more insight on the recency and provides a bigger hover target.

gzdunek · 2024-07-31T16:17:03Z

If we are concerned about the errors length, we can always try the good old text ellipsis:

collapse-expand.mov

The thing I don't like in tooltips is that often they have poor accessibility, especially on touch devices. For example, when it shows on hover, copying its content is impossible:

tooltip.copy.mov

But I'm not insisting on this change, I just wanted to add something to consider.

roraback · 2024-07-31T18:49:05Z

If we are concerned about the errors length, we can always try the good old text ellipsis:

I thought about this as well. The problem I see is that it still is problematic for the layout of a dashboard, particularly where you have two tiles side-by-side. If you expand the one tile, you're either also expanding the tile next to it with a lot of empty space or you're leaving a bunch of empty space below that tile. If we did this kind of behavior, I'd want it to be like the Resources tiles when you overflow the labels—it expands but overlaps the next tile below it until you stop hovering, at which point it snaps back.

I hear you on the accessibility concern though... If copying the error message would be useful, we could make it open on hover, but stay open on click so that if they click on it, it isn't dismissed until they click away.

gzdunek · 2024-08-01T16:32:32Z

I thought about this as well. The problem I see is that it still is problematic for the layout of a dashboard, particularly where you have two tiles side-by-side. If you expand the one tile, you're either also expanding the tile next to it with a lot of empty space or you're leaving a bunch of empty space below that tile. If we did this kind of behavior, I'd want it to be like the Resources tiles when you overflow the labels—it expands but overlaps the next tile below it until you stop hovering, at which point it snaps back.

I see this, thanks for explanation.

I hear you on the accessibility concern though... If copying the error message would be useful, we could make it open on hover, but stay open on click so that if they click on it, it isn't dismissed until they click away.

Sounds good to me.

kimlisa · 2024-08-02T06:05:00Z

@ryanclark @gzdunek

I made the following changes:

allow rows that support status, to be clicked on and applies cursor style pointer f80473a
removed ellipsis on and reordered menu item in dropdown
in enterprise, i made the tooltip sticky https://github.com/gravitational/teleport.e/pull/4784/commits/371cbc0216c1b8b6ed9f808f702ea9a9f4a73194

kimlisa · 2024-08-02T06:36:49Z

web/packages/design/src/Modal/Modal.jsx

@@ -200,6 +200,7 @@ export default class Modal extends React.Component {
          data-testid="Modal"
          ref={this.handleModalRef}
          className={className}
+          onClick={e => e.stopPropagation()}


i couldn't see an issue with this, but the reason why i had to was because of this

the whole row had a click handler, and there is another click handler with options, if i didn't stop the modal propagation, once a user clicks on options and click away from it, it still triggered the rows click handler.

gzdunek · 2024-08-05T10:08:08Z

web/packages/design/src/DataTable/Table.tsx

+      rows.push(
+        <tr
+          key={rowIdx}
+          onClick={row?.onClick ? () => row.onClick(item) : undefined}


Suggested change

onClick={row?.onClick ? () => row.onClick(item) : undefined}

onClick={() => row.onClick?.(item)}

i did as you suggested, but it doesn't matter right that it's still technically calling a function (but nothings happening b/c it's undefined)

gzdunek · 2024-08-05T10:10:35Z

web/packages/design/src/DataTable/Table.tsx

+        <tr
+          key={rowIdx}
+          onClick={row?.onClick ? () => row.onClick(item) : undefined}
+          style={row?.onStyle ? row.onStyle(item) : undefined}


I think getStyle would make more sense here :)

gzdunek · 2024-08-05T10:24:22Z

web/packages/teleport/src/services/integrations/oktastatustypes.ts

+// OktaPluginSyncStatusCode indicates the possible states of an Okta
+// synchronization service.


Can we convert these comments to JSDoc?

gzdunek · 2024-08-05T10:24:50Z

web/packages/teleport/src/services/integrations/oktastatustypes.ts

+// OktaPluginSyncStatusCode indicates the possible states of an Okta
+// synchronization service.
+export enum PluginOktaSyncStatusCode {
+  // is the status code zero value,


I'd explicitly assign numbers to enum values.

gzdunek · 2024-08-05T10:33:28Z

web/packages/teleport/src/services/integrations/oktastatustypes.ts

+  /**
+   * Error contains a textual description of the reason the last synchronization
+   * failed. Only valid when StatusCode is OKTA_PLUGIN_SYNC_STATUS_CODE_ERROR.
+   */


We could enforce this via types:

export type OktaUserSyncDetails = { lastSuccess: Date; lastFailed: Date; enabled: boolean; numUsers: number; } & ( | { statusCode: | PluginOktaSyncStatusCode.Success | PluginOktaSyncStatusCode.Unspecified; } | { statusCode: PluginOktaSyncStatusCode.Error; /** * Contains a textual description of the reason the last synchronization * failed. */ error: string; } );

But I will leave it up to you, it may be a slight overkill for this case.

left it as is

gzdunek · 2024-08-05T10:37:04Z

web/packages/teleport/src/services/integrations/oktastatustypes.ts

+  appFilters: string[];
+  groupFilters: string[];
+  defaultOwners: string[];
+  error: string;


This error is also available only if status code is OKTA_PLUGIN_SYNC_STATUS_CODE_ERROR?

gzdunek · 2024-08-05T10:55:06Z

web/packages/teleport/src/services/integrations/oktastatustypes.ts

oktastatustypes.ts

I see the Go convention, but TBH I'm not sure if it should be applied to JS code. I'd stick with camelCase.

Does the 👍 mean that you wanted to address it? :)

renamed. that was so strange, i thought i renamed it

- Allow on row click handler - Allow custom row styler (func to conditionally apply styles)

gzdunek · 2024-08-07T11:48:33Z

web/packages/teleport/src/Integrations/IntegrationList.tsx

+    history.push(cfg.getIntegrationStatusRoute(row.kind, row.name));
+  }
+
+  function handleOnRowStyle(row: IntegrationLike): React.CSSProperties {


Suggested change

function handleOnRowStyle(row: IntegrationLike): React.CSSProperties {

function getRowStyle(row: IntegrationLike): React.CSSProperties {

gzdunek · 2024-08-07T11:49:10Z

web/packages/teleport/src/Integrations/IntegrationList.tsx

@@ -65,11 +66,27 @@ type Props<IntegrationLike> = {
 type IntegrationLike = Integration | Plugin | ExternalAuditStorageIntegration;

 export function IntegrationList(props: Props<IntegrationLike>) {
+  const history = useHistory();
+
+  function handleOnRowClick(row: IntegrationLike) {


Suggested change

function handleOnRowClick(row: IntegrationLike) {

function handleRowClick(row: IntegrationLike) {

gzdunek · 2024-08-07T11:50:04Z

web/packages/teleport/src/services/integrations/oktastatustypes.ts

Does the 👍 mean that you wanted to address it? :)

public-teleport-github-review-bot · 2024-08-08T22:56:24Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v16	Create PR

github-actions bot added the size/md label Jul 30, 2024

github-actions bot requested a review from gzdunek July 30, 2024 00:27

github-actions bot added the ui label Jul 30, 2024

github-actions bot requested a review from ryanclark July 30, 2024 00:27

kimlisa force-pushed the lisa/okta-status-page branch from 718c4d1 to fcbd5b9 Compare July 30, 2024 00:28

kimlisa changed the title ~~Web: Add plugin status endpoint and its boilerplates (okta)~~ Web: Add plugin status types (okta) and add error kind to ToolTIp.tsx Jul 30, 2024

kimlisa added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Jul 30, 2024

kimlisa force-pushed the lisa/okta-status-page branch from fcbd5b9 to 1dd8cf2 Compare July 30, 2024 00:54

kimlisa force-pushed the lisa/okta-status-page branch from ab982eb to f59c19c Compare August 2, 2024 06:34

kimlisa commented Aug 2, 2024

View reviewed changes

kimlisa force-pushed the lisa/okta-status-page branch 2 times, most recently from 7054b88 to f80473a Compare August 2, 2024 06:50

gzdunek reviewed Aug 5, 2024

View reviewed changes

kimlisa added 5 commits August 6, 2024 20:55

Add error kind to tool tip

9d6b00d

Add status link to okta dropdown in the integraiton list

7ff2456

Add a status route

aa6f05b

Add some types related plug status endpoint

f3ed284

Table add ons and remove ellipsis and re-order

fd72aca

- Allow on row click handler - Allow custom row styler (func to conditionally apply styles)

Address CR

0d72bfe

kimlisa force-pushed the lisa/okta-status-page branch from f80473a to 0d72bfe Compare August 7, 2024 04:12

kimlisa requested a review from gzdunek August 7, 2024 04:14

gzdunek approved these changes Aug 7, 2024

View reviewed changes

ryanclark approved these changes Aug 7, 2024

View reviewed changes

kimlisa force-pushed the lisa/okta-status-page branch from fa5b8b2 to 376ef7d Compare August 7, 2024 16:27

Rename file and func

510bc9e

kimlisa force-pushed the lisa/okta-status-page branch from 376ef7d to 510bc9e Compare August 7, 2024 16:27

kimlisa added this pull request to the merge queue Aug 8, 2024

Merged via the queue into master with commit fff3913 Aug 8, 2024
35 checks passed

kimlisa deleted the lisa/okta-status-page branch August 8, 2024 22:55

kimlisa mentioned this pull request Aug 9, 2024

[v16] Web: Add plugin status types (okta) and add error kind to ToolTIp.tsx #45323

Merged

kimlisa mentioned this pull request Sep 3, 2024

Web Bug Fix: Remove view status button for okta (not fully implemented) #46174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web: Add plugin status types (okta) and add `error` kind to ToolTIp.tsx #44788

Web: Add plugin status types (okta) and add `error` kind to ToolTIp.tsx #44788

kimlisa commented Jul 30, 2024 •

edited

Loading

github-actions bot commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

ryanclark commented Jul 30, 2024 •

edited

Loading

gzdunek commented Jul 30, 2024

roraback commented Jul 31, 2024 •

edited

Loading

roraback commented Jul 31, 2024

gzdunek commented Jul 31, 2024

roraback commented Jul 31, 2024 •

edited

Loading

gzdunek commented Aug 1, 2024

kimlisa commented Aug 2, 2024 •

edited

Loading

kimlisa Aug 2, 2024

gzdunek Aug 5, 2024

kimlisa Aug 7, 2024

gzdunek Aug 5, 2024

gzdunek Aug 5, 2024

gzdunek Aug 5, 2024

gzdunek Aug 5, 2024

kimlisa Aug 7, 2024

gzdunek Aug 5, 2024

gzdunek Aug 5, 2024

gzdunek Aug 7, 2024

kimlisa Aug 7, 2024

gzdunek Aug 7, 2024

gzdunek Aug 7, 2024

gzdunek Aug 7, 2024

public-teleport-github-review-bot bot commented Aug 8, 2024

	onClick={row?.onClick ? () => row.onClick(item) : undefined}
	onClick={() => row.onClick?.(item)}

		// OktaPluginSyncStatusCode indicates the possible states of an Okta
		// synchronization service.

	function handleOnRowStyle(row: IntegrationLike): React.CSSProperties {
	function getRowStyle(row: IntegrationLike): React.CSSProperties {

	function handleOnRowClick(row: IntegrationLike) {
	function handleRowClick(row: IntegrationLike) {

Web: Add plugin status types (okta) and add error kind to ToolTIp.tsx #44788

Web: Add plugin status types (okta) and add error kind to ToolTIp.tsx #44788

Conversation

kimlisa commented Jul 30, 2024 • edited Loading

github-actions bot commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

ryanclark commented Jul 30, 2024 • edited Loading

gzdunek commented Jul 30, 2024

roraback commented Jul 31, 2024 • edited Loading

roraback commented Jul 31, 2024

gzdunek commented Jul 31, 2024

roraback commented Jul 31, 2024 • edited Loading

gzdunek commented Aug 1, 2024

kimlisa commented Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Aug 8, 2024

Web: Add plugin status types (okta) and add `error` kind to ToolTIp.tsx #44788

Web: Add plugin status types (okta) and add `error` kind to ToolTIp.tsx #44788

kimlisa commented Jul 30, 2024 •

edited

Loading

ryanclark commented Jul 30, 2024 •

edited

Loading

roraback commented Jul 31, 2024 •

edited

Loading

roraback commented Jul 31, 2024 •

edited

Loading

kimlisa commented Aug 2, 2024 •

edited

Loading