Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

feat: Workspaces Page Query Filter #1936

wants to merge 2 commits into from

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented May 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.

chrome_i0JLDryS2h

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 f0ssel requested a review from a team as a code owner May 31, 2022 23:01
@f0ssel f0ssel mentioned this pull request May 31, 2022
Closed
@ammario
Copy link
Member

ammario commented May 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.

@f0ssel
Copy link
Contributor Author

f0ssel commented May 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.
  1. Yeah I only cared about updating state, literally 0 time spent on visual.
  2. 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.

@f0ssel
Copy link
Contributor Author

f0ssel commented May 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
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 like map and filter instead of loops, and we're trying to avoid let.

presleyp
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 to useSelector.

@presleyp
Copy link
Contributor

presleyp commented Jun 1, 2022

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.

@misskniss misskniss mentioned this pull request Jun 1, 2022
Closed
90 tasks
@f0ssel f0ssel marked this pull request as draft June 1, 2022 15:31
@f0ssel
Copy link
Contributor Author

f0ssel commented Jun 1, 2022

Nope really just wanted to illustrate the idea, not wanting this code merged

@f0ssel f0ssel closed this Jun 1, 2022
@misskniss
Copy link

This is going to be done via the backend with #1972

@github-actions github-actions bot deleted the f0ssel/workspace-query-filter branch February 4, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@presleyp presleyp presleyp left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@f0ssel @ammario @presleyp @misskniss