NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: Workspaces Page Query Filter#1936

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

f0ssel wants to merge2 commits intomainfromf0ssel/workspace-query-filter

Closed

feat: Workspaces Page Query Filter#1936

f0ssel wants to merge2 commits intomainfromf0ssel/workspace-query-filter

Conversation

Copy link

Contributor

f0ssel commentedMay 31, 2022•
edited
Loading

Discussion at#1820

I wanted to get a proof of concept working so I could hand this off to a more skilled frontend engineer.

I think the big points here I wanted to make are:

Fast implementation
Filter the data we already have, don't bother with backend filtering
Have a sane default query value

f0ssel added2 commits

May 27, 2022 23:35

feat: Add poc for workspace query filer

6e1aaaf

fix query update

139e306

f0ssel requested a review froma team as acode owner

May 31, 2022 23:01

f0ssel mentioned this pull request

May 31, 2022

Workspaces page is difficult to navigate for admin users#1820

Closed

Copy link

Member

ammario commentedMay 31, 2022

Thanks for drafting this up.

Some notes:

The search bar should be left aligned for consistency with the "Create Workspace" button
I think we should deeply consider whether it's implemented on the frontend or the backend. In v1 we decided to put all list filtering and sorting in the frontend to the dismay of our mostly large customers. Then, we never got around to fixing it. We have a fast timeline to generate revenue with this product so expect to rebuild a frontend only implementation within a couple of months.

Copy link

ContributorAuthor

f0ssel commentedMay 31, 2022•
edited
Loading

Thanks for drafting this up.
Some notes:
The search bar should be left aligned for consistency with the "Create Workspace" button
I think we should deeply consider whether it's implemented on the frontend or the backend. In v1 we decided to put all list filtering and sorting in the frontend to the dismay of our mostly large customers. Then, we never got around to fixing it. We have a fast timeline to generate revenue with this product so expect to rebuild a frontend only implementation within a couple of months.

Yeah I only cared about updating state, literally 0 time spent on visual.
Totally, this is dead simple and would be very little to clean up if we wanted to move to backend filtering. I would expect a rework before multi-org since that would require new logic here to filter by orgs. If we wanted something by community MVP I would suggest frontend only like I have here for velocity and effectiveness. Right now all the data for this table is fetched from a single http call.

Copy link

ContributorAuthor

f0ssel commentedMay 31, 2022•
edited
Loading

I'm intentionally trying to get the most out of an impact / effort equation and being aware of the existing timelines and avoiding scope creep.

presleyp reviewed

Jun 1, 2022

View reviewed changes

site/src/pages/WorkspacesPage/WorkspacesPageView.tsx

		const owners: string[] = []
		const newWorkspaces: TypesGen.Workspace[] = []
		const phrases = input.split(" ")
		for (const p of phrases) {

Copy link

Contributor

Choose a reason for hiding this comment

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

We usually use things likemap andfilter instead of loops, and we're trying to avoidlet.

presleyp reviewed

Jun 1, 2022

View reviewed changes

site/src/pages/WorkspacesPage/WorkspacesPage.tsx


		const WorkspacesPage: React.FC = () => {
		const xServices = useContext(XServiceContext)
		const [authState] = useActor(xServices.authXService)
		const { me } = authState.context

Copy link

Contributor

Choose a reason for hiding this comment

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

When you only need one thing out of an xservice, it's better touseSelector.

Copy link

Contributor

Not sure if I'm giving the kind of review you're looking for, let me know! It sounds like you want styling work to follow this up so I'm wondering if we want to merge it soon or hold off - I think what we don't want to do is merge this but not merge style improvements by the Community launch.