Skip to content
This repository was archived by the owner on Jan 8, 2024. It is now read-only.

ui: workspace switcher #2674

Merged
merged 2 commits into from
Nov 12, 2021
Merged

ui: workspace switcher #2674

merged 2 commits into from
Nov 12, 2021

Conversation

jgwhite
Copy link
Contributor

@jgwhite jgwhite commented Nov 4, 2021 •
edited
Loading

Why the change?

Addresses #1339

What’s the plan?

  • Add the switcher
  • Iron out loading states
  • Empty state
  • Add high-level test coverage
  • Add granular test coverage
    • Happy path
    • Error states
    • Loading states
  • Use proper CSS, not cowboy CSS
  • Make dark mode look passable
  • Add changelog entry

What’s not the plan?

Default workspace selection will be addressed in a separate PR. See #2662 for problem statement.

What does it look like?

Switching.Workspace.mp4
Error.Loading.Workspaces.mp4

(note: the loading state no longer appear in the flow above, just the error)

How do I test it?

  1. git checkout ui/workspace-switcher
  2. Boot the dev server in mirage mode or local mode
  3. Navigate to an app page
  4. Try switching workspaces
  5. Verify everything works as you’d expect

@jgwhite jgwhite added the ui label Nov 4, 2021
@jgwhite jgwhite added this to the 0.7.0 milestone Nov 4, 2021
@jgwhite jgwhite self-assigned this Nov 4, 2021
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 803f89c to 1b932e0 Compare November 4, 2021 22:18
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 1b932e0 to 9ea25fc Compare November 4, 2021 22:20
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 9ea25fc to 3d67458 Compare November 5, 2021 15:28
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 3d67458 to 1a295a7 Compare November 5, 2021 15:50
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 1a295a7 to a2dd308 Compare November 8, 2021 13:11
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 734dd16 to c948e21 Compare November 8, 2021 21:34
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 2088604 to 191f58e Compare November 9, 2021 17:29
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 91ed551 to 0df236f Compare November 9, 2021 20:21
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 0df236f to 184003a Compare November 9, 2021 20:40
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 184003a to 7e22b4d Compare November 9, 2021 20:41
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 7e22b4d to a920d41 Compare November 9, 2021 21:03
jgwhite
jgwhite commented Nov 9, 2021
View reviewed changes
ui/app/templates/workspace/projects/project/app.hbs
@@ -9,7 +9,20 @@
)}}
<PageHeader @iconName="git-repository">
<div class="title">
<h1>{{@model.application.application}}</h1>
<div class="title-wrapper">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I entirely needed to introducing a wrapping element here, but I didn’t want to throw off the title styling.

@jgwhite jgwhite marked this pull request as ready for review November 9, 2021 21:41
@jgwhite jgwhite requested review from almonk and a team November 9, 2021 21:42
@amandakalk
Copy link
Contributor

One small thought on the "no active workspaces" state. Assuming a user has not created any workspace and is the default space, maybe we should hide the workspace switcher if you only have a single workspace. Seems like it could cause less confusion if you have no knowledge of workspaces.

I'm also a little confused by the "no active workspaces" message. In your screenshot, isn't "production" considered an active workspace? Should the message say "No other active workspaces found" to be more clear?

@almonk Do you have any thoughts on this?

I don't think this change should hold up this PR though, we can change it later if we agree on a different behavior.

@almonk
Copy link
Contributor

almonk commented Nov 11, 2021

I agree that we shouldn't show the switcher if there's only 1 workspace (and while you can't create them in the UI). @jgwhite what do you think about populating workspace data when the user navigates to an app rather than just in time when the user clicks the dropdown?

sabrinako
sabrinako reviewed Nov 11, 2021
View reviewed changes
ui/tests/integration/components/workspace-switcher-test.ts
import { module, test } from 'qunit';
import { Response } from 'miragejs';

module('Integration | Component | workspace-switcher', function (hooks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Appreciate how comprehensive these tests are!

sabrinako
sabrinako approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@sabrinako sabrinako left a comment

Choose a reason for hiding this comment

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

Looks good to me! For something so complicated under the hood, this code is super straight forward and clear, great job! 🤠 👍

ui/app/components/workspace-switcher.ts
taskFor(this.loadWorkspaces).perform();
}

@task({ restartable: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the first time I'm seeing ember-concurrency and @task, super interesting

@jgwhite
Copy link
Contributor Author

jgwhite commented Nov 11, 2021 •
edited
Loading

@jgwhite what do you think about populating workspace data when the user navigates to an app rather than just in time when the user clicks the dropdown?

@almonk luckily it already does behave that way, I just artificially slowed things down to make the videos more illustrative.

@almonk + @amandakalk I think I’ll go ahead and hide the switcher altogether until we’ve loaded the workspaces and, as you both suggest, only show it under either of these conditions:

  1. There’s more than one workspace
  2. The current workspace is not amongst those returned by the API

@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 85b15c5 to b7d8388 Compare November 11, 2021 21:23
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from b7d8388 to 195bed9 Compare November 12, 2021 10:38
@jgwhite @izaaklauer
@almonk @amandakalk
ui: add workspace switcher
9c89ad5 
Co-authored-by: Izaak Lauer <[email protected]>
Co-authored-by: Alasdair Monk <[email protected]>
Co-authored-by: Amanda Cahill <[email protected]>
@jgwhite jgwhite force-pushed the ui/workspace-switcher branch from 195bed9 to 9c89ad5 Compare November 12, 2021 11:39
@jgwhite jgwhite merged commit f556435 into main Nov 12, 2021
@jgwhite jgwhite deleted the ui/workspace-switcher branch November 12, 2021 11:51
@jgwhite jgwhite mentioned this pull request Nov 12, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sabrinako sabrinako sabrinako approved these changes

@almonk almonk Awaiting requested review from almonk

Assignees

@jgwhite jgwhite

Labels
ui
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jgwhite @amandakalk @almonk @sabrinako