Skip to content

Conversation

@svenefftinge
Copy link
Contributor

@svenefftinge svenefftinge commented Mar 18, 2021

Fixes outstanding todos from #3426
Namely:

  • implement search
  • update pinned and shareable
  • implement pagination (more button on bottom)
  • remove status: prefix from filter
  • fix ordering
  • Implement "No Active Workspaces" screen
  • Show GC warning

@svenefftinge svenefftinge requested a review from gtsiolis March 18, 2021 10:34
@svenefftinge svenefftinge force-pushed the se/more-workspaces branch 4 times, most recently from 634e0b1 to 675348d Compare March 18, 2021 15:05
Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks @svenefftinge! First round of comments are in! Feel free to leave anything as a follow up issue. 🥊

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: What do you think of making the link more subtle here?

Suggested change
<div className="text-center pb-6">Prefix a git repository URL with gitpod.io/# or open a workspace template. <a className="text-blue underline" href="https://www.gitpod.io/docs/getting-started/">Learn how to get started</a></div>
<div className="text-center pb-6 text-gray-500">Prefix a git repository URL with gitpod.io/# or open a workspace template. <a className="text-gray-400 underline underline-thickness-thin underline-offset-small hover:text-gray-600" href="https://www.gitpod.io/docs/getting-started/">Learn how to get started</a></div>

Copy link
Contributor

Choose a reason for hiding this comment

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

issue: We're using only two radius variants across the product. Buttons, text fields, and tabs (sidebar, topbar) should use 6px radius (rounded-md). Modals, Lists (Workspaces), etc should use 12px radius (rounded-xl).

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I think it's ok to remove the prefix from within the list options and leave here there only the values like 50, 100, 200. The dropdown title can then be Items per page: X or Workspaces: X although there's one single page for the time being. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about Limit: 50?

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: The initial thought was to remove the prefix from within the dropdown options but keep the prefix on the title here to make the component-filter more clear. This could still remain Status: Active.

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Since we're here let's also change this to something like the following:

Suggested change
<Header title="Workspaces" subtitle="Manage past workspaces" />
<Header title="Workspaces" subtitle="Manage all workspaces and see pending changes." />

Subtitle is also $gray-500 (#78716C).

Copy link
Contributor

@gtsiolis gtsiolis Mar 18, 2021

Choose a reason for hiding this comment

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

issue: Opening in a new tab by holding CMD does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Search works like a charm! 🔮

Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Let's use the same plugin for the underline here.

Suggested change
<p className="text-gray-500">Unpinned workspaces that have been stopped for more than 14 days will be automatically deleted. <a className="text-blue-500 underline" href="https://www.gitpod.io/docs/life-of-workspace/#garbage-collection">Learn more</a></p>
<p className="text-gray-500">Unpinned workspaces that have been stopped for more than 14 days will be automatically deleted. <a className="text-blue-600 underline underline-thickness-thin underline-offset-small hover:text-gray-800">Learn more</a></p>

Copy link
Contributor

Choose a reason for hiding this comment

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

thought: X Changes below with the text-gitpod-kumquat color seems like. Maybe let's go with a red one. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

issue: The garbage collection message should remain when searching All workspaces. In the future we could deselect a filter or have a more clear searxh state when searching and remove the GC message. For example, the GV message could include the state of the search (e.g Search Results for xyz). 🔍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe when I search for a specific workspace, I have already seen the warning and really would like to be able to focus on the search result. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree but I'd say it depends on if we hold or release the filter state on the status.

thought: For example, if the filter status persists and search happens only for the All workspaces it wouldn't hurt to persist the GC message. If on search, we release the filter state and search across all workspaces it would make more sense to remove the GC message. Since the second filter option is All here it's easier to remove the GC message here. Let's do it!

@gtsiolis
Copy link
Contributor

gtsiolis commented Mar 18, 2021

/werft run

👍 started the job as gitpod-build-se-more-workspaces.6

@svenefftinge svenefftinge removed the request for review from AlexTugarev March 18, 2021 16:56
@svenefftinge svenefftinge merged commit d644c6b into dashboard-v2 Mar 18, 2021
@svenefftinge svenefftinge deleted the se/more-workspaces branch March 18, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants