Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Workspaces filtering #1972

Merged
merged 23 commits into from
Jun 3, 2022
Merged

feat: Workspaces filtering #1972

merged 23 commits into from
Jun 3, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Jun 1, 2022

Okay third time is the charm. This one I'd actually like to get merged.

Discussion at #1820

This PR adds a workspace filter to the workspaces page. It uses the backend and it defaults to the workspaces you own, and there's buttons to see all workspaces and a filter bar. Right now the filter bar only functions with the owner: query filter.

chrome_zNVFzNQXaq

Next steps:

  • Work with / hand off to a frontend person to help with styling issues
  • Correctly handle 400 error cases where we should show no workspaces
  • Support plain words as a workspace name filter (requires backend changes, probably different PR)

@f0ssel f0ssel requested a review from a team as a code owner June 1, 2022 23:39
@f0ssel
Copy link
Contributor Author

f0ssel commented Jun 1, 2022

I'd like help if someone with styling experience wants to fix the issues with sizing / alignment between the button and filter field and the length of the text field itself. I'm trying to emulate the look and feel of the issue query bar on Github, for example:
image

site/src/util/workspace.test.ts Outdated Show resolved Hide resolved
site/src/util/workspace.ts Outdated Show resolved Hide resolved
site/src/util/workspace.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

Can you please add all product copy into a Language object (example the words "Your workspaces" and "All workspaces" in the WorkspacePage

site/src/pages/WorkspacesPage/WorkspacesPage.tsx Outdated Show resolved Hide resolved
site/src/pages/WorkspacesPage/WorkspacesPage.tsx Outdated Show resolved Hide resolved
site/src/pages/WorkspacesPage/WorkspacesPage.tsx Outdated Show resolved Hide resolved
site/src/pages/WorkspacesPage/WorkspacesPage.tsx Outdated Show resolved Hide resolved
site/src/pages/WorkspacesPage/WorkspacesPage.tsx Outdated Show resolved Hide resolved
site/src/pages/WorkspacesPage/WorkspacesPage.tsx Outdated Show resolved Hide resolved
site/src/pages/WorkspacesPage/WorkspacesPage.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

Nicely done @f0ssel . All code comments are really nit, this is fantastic.

@ammario
Copy link
Member

ammario commented Jun 2, 2022

Yay for the (scalable) backend implementation.

What should be/is the behavior if no term is provided? For example, if I just searched "ammario" and not "owner:ammario" what should happen?

@f0ssel
Copy link
Contributor Author

f0ssel commented Jun 2, 2022

Yay for the (scalable) backend implementation.

What should be/is the behavior if no term is provided? For example, if I just searched "ammario" and not "owner:ammario" what should happen?

Right now that's not implemented so it's just ignored. I want to make plain strings (no owner: prefix) to just do a search on workspace name, but that isn't supported on the backend yet.

@ammario
Copy link
Member

ammario commented Jun 2, 2022

I think that's right @f0ssel. The interesting thing is the search could be against the fully qualified workspace name, so you technically use that to search for owner as well.

@f0ssel
Copy link
Contributor Author

f0ssel commented Jun 2, 2022

@ammario I added this suggestion, why don't you take a look at this logic and let me know if you would expect different behavior: https://github.com/coder/coder/pull/1972/files#diff-46ce05d14402854a3999da903dcf3949351172316989f053196857f93fcc6d57R198-R229

@ammario
Copy link
Member

ammario commented Jun 2, 2022

@ammario I added this suggestion, why don't you take a look at this logic and let me know if you would expect different behavior: https://github.com/coder/coder/pull/1972/files#diff-46ce05d14402854a3999da903dcf3949351172316989f053196857f93fcc6d57R198-R229

That was a fun exercise of some of my older neurons. That LGTM! I'm excited to see this in action.

f0ssel and others added 2 commits June 2, 2022 21:13
Co-authored-by: G r e y <grey@coder.com>
@f0ssel
Copy link
Contributor Author

f0ssel commented Jun 2, 2022

Will be getting some styling fixes in soon, but here's a demo of the current functionality:
chrome_BgiGT3ZExx

@ammario
Copy link
Member

ammario commented Jun 2, 2022

I like the UX. The flicker is a bit jank, but I know you realize that too.

@tjcran tjcran mentioned this pull request Jun 3, 2022
@f0ssel
Copy link
Contributor Author

f0ssel commented Jun 3, 2022

After some awesome styling work from @Kira-Pilot I think this is ready to merge, will be tagging for final reviews.

chrome_8JHeeyht9E

@f0ssel f0ssel requested review from a team and ammario June 3, 2022 16:17
@Kira-Pilot
Copy link
Member

great job braving the FE!

coderd/database/queries/workspaces.sql Outdated Show resolved Hide resolved
coderd/workspaces.go Show resolved Hide resolved
@f0ssel f0ssel enabled auto-merge (squash) June 3, 2022 17:10
@f0ssel f0ssel merged commit 8b03e2b into main Jun 3, 2022
@f0ssel f0ssel deleted the f0ssel/workspace-filter branch June 3, 2022 17:20
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
Co-authored-by: G r e y <grey@coder.com>
Co-authored-by: Kira Pilot <kira@coder.com>
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.

None yet

6 participants