Skip to content

[Web] Add List View to Unified Resources#34203

Merged
rudream merged 1 commit intomasterfrom
yassine/unified-list
Nov 10, 2023
Merged

[Web] Add List View to Unified Resources#34203
rudream merged 1 commit intomasterfrom
yassine/unified-list

Conversation

@rudream
Copy link
Copy Markdown
Contributor

@rudream rudream commented Nov 3, 2023

Purpose

Adds a list view option to the Unified Resources page as an alternative to the card view.

Also changes the hover state for the card view to use a shadow.

e counterpart: https://github.com/gravitational/teleport.e/pull/2619

Figma design

Demo

image

TODO:

changelog: Add list view to resources page

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 3, 2023

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
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 3, 2023

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.

@rudream rudream force-pushed the yassine/unified-list branch 3 times, most recently from a4d084a to cc41cb6 Compare November 3, 2023 19:18
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 3, 2023

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.

@rudream rudream force-pushed the yassine/unified-list branch from cc41cb6 to 900f096 Compare November 3, 2023 21:16
Comment thread web/packages/shared/components/UnifiedResources/FilterPanel.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/FilterPanel.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/FilterPanel.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/types.ts Outdated
Comment thread web/packages/shared/components/UnifiedResources/types.ts Outdated
Comment thread web/packages/teleport/src/UnifiedResources/UnifiedResources.tsx Outdated
Comment thread web/packages/teleport/src/services/localStorage/types.ts Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
@gzdunek
Copy link
Copy Markdown
Contributor

gzdunek commented Nov 6, 2023

Something is causing the the horizontal scrollbar blinking (Firefox):

horizontal.scroll.mov

@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Nov 6, 2023

What do you think about putting a tooltip over the labels icon that says "Show Labels" (similar to how we show the "Click to Copy" tooltip on the copy button)?

@rudream rudream force-pushed the yassine/unified-list branch from 900f096 to c8129a3 Compare November 8, 2023 03:23
@rudream
Copy link
Copy Markdown
Contributor Author

rudream commented Nov 8, 2023

Something is causing the the horizontal scrollbar blinking (Firefox)

@gzdunek Good catch, looks like this was being caused because the box-shadow on hover was increasing the width by a few pixels. I added a workaround using a pseudo-element to prevent this.

@rudream rudream requested a review from gzdunek November 8, 2023 05:04
Comment thread web/packages/shared/components/ToolTip/HoverTooltip.tsx
@avatus
Copy link
Copy Markdown
Contributor

avatus commented Nov 8, 2023

This looks/works great. My only concern is leaving out the user preferences part because until that goes in, a user who would want this view would have to click the list view button every time they load the page (could lead to more frustration than not having it at all). Lets pair up and bust it out real quick to squeeze it into this PR?

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

Second pass

Comment thread web/packages/shared/components/ToolTip/HoverTooltip.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/CardsView/CardsView.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/shared/utils.ts Outdated
Comment thread web/packages/shared/components/UnifiedResources/shared/utils.ts
Comment thread web/packages/shared/components/UnifiedResources/shared/utils.ts
Comment thread web/packages/shared/components/UnifiedResources/shared/index.ts
@rudream rudream force-pushed the yassine/unified-list branch from e54aa45 to baef29c Compare November 9, 2023 06:23
@rudream rudream requested review from avatus and gzdunek November 9, 2023 07:20
@rudream rudream force-pushed the yassine/unified-list branch 2 times, most recently from a762d03 to d0bc2d6 Compare November 9, 2023 09:33
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

We are almost there!

Comment thread web/packages/shared/components/ToolTip/HoverTooltip.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment on lines +288 to +289
const selectViewMode = (viewMode: UnifiedViewModePreference) => {
updateUnifiedResourcesPreferences({
...unifiedResourcePreferences,
viewMode,
});
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.

This is great 👏
However, now we have some inconsistency: viewMode state is kept outside, but information if pinned tab is active is kept in params and sent externally via updateUnifiedResourcesPreferences.
It seems to me that it would be good to have a single way of updating both properties. I made an attempt to refactor this, you can check out the draft #34394.
It's not perfect, but we should refactor useUrlFiltering first.
I also improved naming and fixed imports (shared and teleterm shouldn't import from teleport).

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.

Agreed, though I think we should do the refactoring after this is merged since this PR is already large enough and this doesn't affect UX.

Comment thread web/packages/shared/components/UnifiedResources/types.ts
@rudream rudream requested a review from gzdunek November 10, 2023 00:08
Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

Looks and works great to me. 👍 , after @gzdunek comments are taken care of

Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

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

I left two small comments.

🚢 it!

Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/ListView/ResourceListItem.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/shared/PinButton.tsx Outdated
@rudream rudream force-pushed the yassine/unified-list branch from 4388f04 to af5e440 Compare November 10, 2023 16:24
@rudream rudream enabled auto-merge November 10, 2023 16:24
@rudream rudream force-pushed the yassine/unified-list branch 6 times, most recently from 9ac71a7 to d0b42dc Compare November 10, 2023 18:32
@rudream rudream force-pushed the yassine/unified-list branch from d0b42dc to 21d6550 Compare November 10, 2023 18:49
@rudream rudream added this pull request to the merge queue Nov 10, 2023
Merged via the queue into master with commit 99ae674 Nov 10, 2023
@rudream rudream deleted the yassine/unified-list branch November 10, 2023 19:25
@public-teleport-github-review-bot
Copy link
Copy Markdown

@rudream See the table below for backport results.

Branch Result
branch/v14 Failed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants