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

revise recent projects flow to be less confusing#464

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

Conversation

bcpeinhardt
Copy link
Collaborator

@bcpeinhardtbcpeinhardt commentedAug 14, 2024
edited
Loading

Edit: we changed the flow, links do show but are disabled if the workspace is in a non-starting pending state, and we started the workspace automatically negating the need to even have the manual controls

Previously recent workspaces had a status icon which looked a bit like a button to push, and the start workspace button could be confused as the cta to open a project.
Now the project links in recent projects only display once the workspace is running and ready to accept connections, and the confusing icon has been removed.

I think this flow is pretty clear, but people used to just clicking the link might freak out when they don't see their links or be frustrated they need to press two buttons.
An alternative option could be to remove the workspace start button entirely, since following a project link should start the workspace if required.

Screen.Recording.2024-08-14.at.12.06.40.PM.mov

resolves#436

EDIT:

We went down a rabbithole and reworked the recents page to be more consistent with the VSCode extension. There are no longer indpendent workspace controls on the recents page. You simply click the link and it handles the workspace lifecycle for you.

github-actions[bot] reacted with thumbs up emoji
@github-actionsGitHub Actions
Copy link
Contributor

github-actionsbot commentedAug 14, 2024
edited
Loading

Qodana Community for JVM

4 new problems were found

Inspection nameSeverityProblems
Incorrect string capitalization🔶 Warning1
Usage of redundant or deprecated syntax or deprecated symbols🔶 Warning1
Redundant nullable return type🔶 Warning1
String concatenation that can be converted to string template◽️ Notice1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register atQodana Cloud andconfigure the action
  2. UseGitHub Code Scanning with Qodana
  3. HostQodana report at GitHub Pages
  4. Inspect and useqodana.sarif.json (seethe Qodana SARIF format for details)

To get*.log files or any other Qodana artifacts, run the action withupload-result option set totrue,
so that the action will upload the files as the job artifacts:

      -name:'Qodana Scan'uses:JetBrains/qodana-action@v2023.3.2with:upload-result:true
Contact Qodana team

Contact us atqodana-support@jetbrains.com

@bcpeinhardtbcpeinhardt marked this pull request as ready for reviewAugust 16, 2024 19:04
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.

Nicely done!

}.topGap(gap)
if (deploymentError == null || showError) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to add this check back or something similar to it. It is poorly named but basically it was making sure we only display an API error on the first workspace rather than duplicating it on each workspace (it was pretty noisy, especially since the API errors can get long).

For example, say you have 5 workspaces and your token expires, you see a message about being unauthorized and to check your token 5 times on the page.

Or we could group workspaces under a deployment heading and show the API error under the deployment heading, but that might be out of scope for this PR.

Copy link
CollaboratorAuthor

@bcpeinhardtbcpeinhardtAug 19, 2024
edited
Loading

Choose a reason for hiding this comment

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

It looks like atm the moment I actually have this inside the connections.foEach loop (which makes me think with multiple WorkpaceProjectIDE combos it would render multiple times). I didn't notice because I was visually testing with one recent connection.
I'll pull it out of the loop and add the check.

^^^ My adhd is going to quickly become a driving force for building out E2E suites.

code-asher reacted with rocket emoji
Comment on lines 209 to 211
if (inLoadingState) {
icon(AnimatedIcon.Default())
}
Copy link
Member

Choose a reason for hiding this comment

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

Not at all a blocker and completely optional, more just idly musing, but I wonder if we want to turn the pair back into a triple and put a nullable icon in there, which would let us also show a spinner while the query is running as well (like it used to) and bring back the error icon for API errors (if we want, honestly the red color is probably sufficient).

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

I think adding the error icon back is a good idea and fits in this PR :)

Comment on lines +224 to +225
if (listOf(WorkspaceStatus.STOPPED, WorkspaceStatus.CANCELED, WorkspaceStatus.FAILED).contains(workspace.latestBuild.status)) {
client.startWorkspace(workspace)
Copy link
Member

Choose a reason for hiding this comment

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

I opened#465 for future improvement

bcpeinhardt reacted with rocket emoji
@code-asher
Copy link
Member

Oh forgot, we should also update the changelog and make sure we describe when the links are enabled and that the manual controls are replaced in favor of automatically starting the workspace.

bcpeinhardt reacted with thumbs up emoji

@Kira-Pilot
Copy link
Member

resolves#436

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!

Comment on lines 214 to 216
if (status.third != null) {
icon(status.third!!)
}
Copy link
Member

Choose a reason for hiding this comment

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

idk if this is more Kotlin-y but I think you can also do something like this over the assertion:

status.third?.let { icon (it) }

Admittedly untested though

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

I low key hate that they named a functionlet 💀 TBH I think the assertion here is clearer, but if that's the kotlin way then I'm game haha. I could also do awithoutNull I suppose.

Copy link
Member

@code-ashercode-asherAug 19, 2024
edited
Loading

Choose a reason for hiding this comment

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

withoutNull throws if null but here we are OK with the status potentially being null, so I think it would not quite fit here.

I think the main issue with assertions is that they bypass the safety provided to you by the language, which feels dangerous for an unclear benefit.

If you see a!! you need to reason about the entire code and determine thatstatus.third is not going to be reassigned (and you need to do this every time related code is refactored), but we should let the machine do it for us. 😆 Admittedly it is pretty trivial in this case, but still!

JBou reacted with thumbs up emoji
Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Yeah I meant use withoutNull inside the if check, but you're totally right thelet method is the best option here, I just wish they called it somewhere else lol.

code-asher reacted with thumbs up emojicode-asher 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.

ahaha yeah and I swear I have to look it up every time because I always forget it is calledlet

@bcpeinhardtbcpeinhardt merged commitd2f50f1 intomainAug 19, 2024
6 checks passed
@bcpeinhardtbcpeinhardt deleted the bcpeinhardt/436-add-labels-to-workspace-actions branchAugust 19, 2024 22:26
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@code-ashercode-ashercode-asher approved these changes

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

Successfully merging this pull request may close these issues.

Add labels to workspace actions
3 participants
@bcpeinhardt@code-asher@Kira-Pilot

[8]ページ先頭

©2009-2025 Movatter.jp