Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: Redesign workspaces page#1450

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

Merged
kylecarbs merged 24 commits intomainfromworkspacepage
May 16, 2022
Merged

feat: Redesign workspaces page#1450

kylecarbs merged 24 commits intomainfromworkspacepage
May 16, 2022

Conversation

kylecarbs
Copy link
Member

@kylecarbskylecarbs commentedMay 14, 2022•
edited
Loading

image

As part of redesigning the workspaces page, I updated the theme to dark mode and removed overrides resolving#1449. There's a lot of work that can be done here to improve, but this is a good start!

I've checked all pages to ensure nothing is bugged/broken, and it all looks good!

A text input will be added in the future to allow for filtering. This will work similarly to GitHub's issue filters:owner:kylecarbs,status:running,is:outdated, etc.

I proposed initially that workspaces be nested under templates, but that provided a restrictive view for admins. Admins and managers should get a full view of their Coder deployment from the workspaces page, which they now can!

Todo

  • Write some tests
  • Remove the old flow via templates
  • Empty view 🔍
  • Get some feedback on the UI
  • Maybe pagination? Might do that in another PR

greyscaled reacted with rocket emoji
@kylecarbskylecarbs requested review frompresleyp anda team ascode ownersMay 14, 2022 03:07
Copy link
Member

@code-ashercode-asher 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 so far! All the pages look great but I think something needs to be updated for the stories since they are all throwing errors.

content: `" "`,
bottom: 0,
left: theme.spacing(3),
background: "#C16800",
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good theme color that could go here in place of the hardcoded color? Likesecondary or something maybe.

presleyp reacted with thumbs up emoji
@@ -61,8 +61,9 @@ const config: Configuration = {
port: process.env.PORT || 8080,
proxy: {
"/api": {
target: "http://localhost:3000",
target: "https://dev.coder.com",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an environment variable or something so we can still develop locally?

kylecarbs reacted with thumbs up emoji
@greyscaled
Copy link
Contributor

@code-asher@kylecarbs

I think something needs to be updated for the stories since they are all throwing errors.

The theme provider for storybook needs to be set in.storybook/preview.js --> we can modify the below to only accept dark theme. I didn't test that locally, but it's a start as to where to look.

import{dark,light}from"../src/theme"
import"../src/theme/globalFonts"
constproviderFn=({ children, theme})=>(
<ThemeProvidertheme={theme}>
<CssBaseline/>
{children}
</ThemeProvider>
)
addDecorator(withThemes(null,[light,dark],{ providerFn}))

code-asher and kylecarbs reacted with heart emoji

@@ -25,7 +25,7 @@
"typegen": "xstate typegen 'src/**/*.ts'"
},
"dependencies": {
"@fontsource/fira-code": "4.5.9",
"@fontsource/ibm-plex-mono": "^4.5.9",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Can you pin this dependency to4.5.9? You run theupgrade command to do it:

cd siteyarn upgrade @fontsource/ibm-plex-mono@4.5.9

Copy link
Member

Choose a reason for hiding this comment

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

@vapurrmaid I notice we haveyarn.lock - what's the thought process behind pinning dependencies?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll answer those two questions separately:

  • Why do we want them pinned

vs

  • Why pin in package.json

For the former: We have a RFC and guide covering, but the TL:DR is more incremental upgrades w depend-a-bot
For the latter: I don't know if having them unpinned interferes with depend-a-bot or any other tool, but simply for precision. I shouldn't have to hunt down the actual version inyarn.lock to answer what version of X we're on.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, will look at the guide!

Comment on lines 39 to 43
<TableCell>Name</TableCell>
<TableCell>Template</TableCell>
<TableCell>Version</TableCell>
<TableCell>Last Built</TableCell>
<TableCell>Status</TableCell>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: All of these should be defined inLanguage, including the<Button> text above

Copy link
Member

Choose a reason for hiding this comment

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

Where did theLanguage convention come from? Have we thought about usingreact-i18next?

Copy link
Contributor

@greyscaledgreyscaledMay 16, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Great question!

i18n was not implemented in v1, and will be an eventual need for v2. We haven't yet planned for when but we know it will be an effort that we will eventually take on. In the meantime,Language is a simple way of using primitives to begin resourcing strings for what's next. The idea wasn't to commit to a framework or have to solve that problem, but ensure that we're organized and ready to for when we do.

As a bonus, we can shareLanguage in test like this:

it("renders the sign-in form",async()=>{
// When
render(<LoginPage/>)
// Then
awaitscreen.findByText(Language.passwordSignIn)
})

Kira-Pilot reacted with heart emoji
Copy link
Member

Choose a reason for hiding this comment

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

Great, make sense to me! Thanks for explaining.

@greyscaled
Copy link
Contributor

Praise I really like the Status column, it's very nice 🎨

@BrunoQuaresmaBrunoQuaresma mentioned this pull requestMay 16, 2022
7 tasks
@BrunoQuaresma
Copy link
Collaborator

@kylecarbs I assigned this ticket#766 to you since this work looks to be related to it.

kylecarbs reacted with thumbs up emoji

@Kira-Pilot
Copy link
Member

I noticed we're keeping some overrides intheme/overrides.ts - is removing these work leftover that should be completed at a later time, or do we need to keep this stuff?

@kylecarbs
Copy link
MemberAuthor

@Kira-Pilot those are things I added specifically in this PR to adjust some stylistic things.material-uimostly worked for our needs, but I just tweaked a few smaller visual items.

Kira-Pilot reacted with thumbs up emoji

@@ -0,0 +1,32 @@
export const getTimeSince = (date: Date): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use a library for this time stuff instead? I've used moment.js and day.js in the past

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yup, I implemented@vapurrmaid's suggestion of usingday.js!

Copy link
Contributor

@greyscaledgreyscaledMay 16, 2022•
edited
Loading

Choose a reason for hiding this comment

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

I made this suggestion above:#1450 (comment) which is prior art from v1. I'm quite happy with dayjs from that experience, and I know it's a small library.

@Kira-Pilot
Copy link
Member

@kylecarbs I am having a hard time running this locally using./develop.sh.@presleyp was struggling as well. Any idea what we might be missing?

@BrunoQuaresma
Copy link
Collaborator

Doing some QA I was not able to create a new workspace from the UI:
Screen Shot 2022-05-16 at 14 36 40

Is it expected?

@kylecarbs
Copy link
MemberAuthor

Yup! The create workspace page is included in#801

Copy link
Member

@code-ashercode-asher left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I like the default look of the field input labels. The dialogs looked a bit better before IMO but definitely still usable (I like the reset password dialog though).

The margins component might need a background color removed or something, right now it has white text on a gray backgroundhttps://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=62828332ad84ca004a3e0e03

Very handy we can see all the different workspace states in Chromatic!

Edit: this one seemed a bit strange to me as well, is that a disabled button?https://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=62828332ad84ca004a3e0ded
Not sure if this matters but should this be white?https://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=62828332ad84ca004a3e0dfd

kylecarbs reacted with thumbs up emoji
Copy link
Contributor

@greyscaledgreyscaled left a comment

Choose a reason for hiding this comment

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

One quick question I thought I'd submit review for right away since it's simple.

Following up with other items shortly.

@@ -1,20 +1,17 @@
import CssBaseline from "@material-ui/core/CssBaseline"
import ThemeProvider from "@material-ui/styles/ThemeProvider"
import { withThemes } from "@react-theming/storybook-addon"
Copy link
Contributor

@greyscaledgreyscaledMay 16, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Question(if-minor): Are we able to remove this dependency now"@react-theming/storybook-addon": "1.1.5", ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I think so!

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Good catch.


export const Language = {
createButton: "Create Workspace",
emptyView: "so you can check out your repositories, edit your source code, and build and test your software.",
Copy link
Contributor

Choose a reason for hiding this comment

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

@kylecarbs
Copy link
MemberAuthor

@code-asher all have been fixed! That weird button is actually a loading button, and looked weird before!

code-asher reacted with hooray emoji

@@ -9,7 +9,7 @@ export default {

const Template: Story = (args) => (
<Margins {...args}>
<div style={{ width: "100%", background: "lightgrey" }}>Here is some content that will not get too wide!</div>
<div style={{ width: "100%" }}>Here is some content that will not get too wide!</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs a background color - it proves that the margins are working

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ahh good catch! I'll make it black.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed!

import { WorkspacesPageView } from "./WorkspacesPageView"

const WorkspacesPage: React.FC = () => {
const [workspacesState] = useMachine(workspacesMachine)
Copy link
Contributor

Choose a reason for hiding this comment

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

This reminds me that now that I'm doing a bunch of polling on the workspace page, I can and shoulduseMachine for that too.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oh interesting. I never thought of that, but I suppose it makes sense.

Copy link
Contributor

@presleyppresleyp 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, I have a couple important but small comments about the status bar and then some stylistic things you can decide on

@kylecarbs
Copy link
MemberAuthor

Thank you for the reviews everyone 🥳🥳🥳🥳🥳

@vapurrmaid I see you're pending on UI for Storybook. I'm going to merge this in, but I'mextremely happy to make any changes post!

code-asher reacted with hooray emoji

@kylecarbskylecarbs merged commit22ec366 intomainMay 16, 2022
@kylecarbskylecarbs deleted the workspacepage branchMay 16, 2022 21:52
@misskniss
Copy link

misskniss commentedMay 17, 2022•
edited
Loading

fixes#699
fixes#766

kylecarbs added a commit that referenced this pull requestJun 10, 2022
* feat: Improve navbar to be more compactThe navbar was unnecessarily large before, which madethe UI feel a bit bloaty from my perspective.* Attempt to remove overrides* Update theme* Add text field* Update theme to dark!* Fix import ordering* Fix page location* Fix requested changes* Add storybook for workspaces page view* Add empty view* Add tests for empty view* Remove templates page* Fix local port* Remove templates from nav* Fix e2e test* Remove time.ts* Remove dep* Add background color to margins* Merge status checking from workspace page* Fix requested changes* Fix workspace status tests
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@greyscaledgreyscaledgreyscaled left review comments

@presleyppresleyppresleyp approved these changes

@Kira-PilotKira-PilotKira-Pilot approved these changes

@code-ashercode-ashercode-asher approved these changes

Assignees

@kylecarbskylecarbs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@kylecarbs@greyscaled@BrunoQuaresma@Kira-Pilot@misskniss@presleyp@code-asher
[8]ページ先頭
©2009-2025 Movatter.jp