Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ide] added VS Code "Insiders" IDE variant #5024

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

JanKoehnlein
Copy link
Contributor

Fixes #4965

@JanKoehnlein
Copy link
Contributor Author

JanKoehnlein commented Aug 5, 2021

It's a bit stupid to test, as we have no new commits to gitpod/vscode:gp-code since the last stable build, so both options yield the same vscode version. I tested by setting the branch of the not yet merged PR https://github.com/gitpod-io/vscode/pull/22/files in the IDE dockerfile. We may want to revert that before merging.

To test:

  • Set "VS Code" as IDE in the settings
  • Start a workspace
  • F1 "Chat" should not find the "Gitpod: Open Community Chat" command
  • Set "VS Code Insiders" as IDE
  • Start a new workspace
  • F1 "Chat" should find the "Gitpod: Open Community Chat" command

So it basically works. Any proposal to improve the display of the IDE buttons on the Settings page is welcome. At them moment "VS Code Insiders" is trimmed to "VS Code I...". Maybe we should call it "VS Code ß" (beta) or so.

@JanKoehnlein JanKoehnlein marked this pull request as ready for review August 5, 2021 19:23
@akosyakov
Copy link
Member

akosyakov commented Aug 6, 2021

/werft run

👍 started the job as gitpod-build-jankoehnlein-code-preview-version-4965.2

@akosyakov
Copy link
Member

/hold

because of test commit

@akosyakov
Copy link
Member

Rendering is off:
Screenshot 2021-08-06 at 08 10 22

Does it make sense to add some warning when one selects Insider, explaining what it is?

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Hi everyone! Left two minor comments below. Cc @JanKoehnlein @akosyakov

A boring (simple) solution here could be to use the same card components, remove the Insiders suffix and use a label at the bottom of the VS Code Insiders option like LATEST, INSIDERS, or UNSTABLE.

Example (Latest) Example (Insiders)
1 2

In case this is helpful, the label component has been used in the What's New modal[1] and the upcoming Project Branches page[2]. 💡

🍊 🍊 🍊 🍊

In future iterations we could consider redesigning this component to be more flexible. Here's an early draft design how this could look like:

Early Draft
Preferences

🍋 🍋 🍋 🍋

Does it make sense to add some warning when one selects Insider, explaining what it is?

I think the label inside the card option could suffice. Additionally, we could update the section subtitle to be more accurate:

From:

Choose which IDE you want to use

To something like:

Choose an editor between VS Code, VS Code Insiders, and Theia.

<img className="w-16 filter-grayscale" src={vscode}/>
</div>
</SelectableCard>
<SelectableCard className="w-36 h-40" title="VS Code Insiders" selected={defaultIde === 'code-insiders'} onClick={() => actuallySetDefaultIde('code-insiders')}>
<div className="flex-grow flex justify-center items-center">
<img className="w-16 filter-grayscale" src={vscode}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: What do you think of using the actual VS Code Insider logo here which is slightly different? This could help regular VS Code users identify faster the difference between these two options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not such a big friend of the name "Insiders", because it's the latest port of our changes to the latest release of VS Code rather than the official VS Code Insiders (which is ahead of the current VS Code release). Do you think that matters or is that nitpicking?

Copy link
Contributor

@gtsiolis gtsiolis Aug 6, 2021

Choose a reason for hiding this comment

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

Oh, wait. Maybe I've missed the context here. I thought this was about having the option to switch to the official VS Code Insiders as the editor option. If this is about our changes on top of VS Code I'd vote for definitely removing the term Insiders altogether as it could be misleading.

Using a more user friendly term like Latest or Unstable sound more appropriate here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about choosing between "VS Code stable" between "VS Code latest"?
Who would choose an unstable IDE?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the Latest label sounds better here. I think we don't have to use any label for the Stable option within the product.

<img className="w-16 filter-grayscale" src={vscode}/>
</div>
</SelectableCard>
<SelectableCard className="w-36 h-40" title="VS Code Insiders" selected={defaultIde === 'code-insiders'} onClick={() => actuallySetDefaultIde('code-insiders')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: To make this option friendlier to most users which could be unfamiliar with VS Code Insiders we could use a label here to mark this option as the latest or unstable option. 💭

@akosyakov
Copy link
Member

We should probably also think how we indicate it in within IDE.

@JanKoehnlein
Copy link
Contributor Author

JanKoehnlein commented Aug 9, 2021

/werft run

👍 started the job as gitpod-build-jankoehnlein-code-preview-version-4965.3

@JanKoehnlein
Copy link
Contributor Author

JanKoehnlein commented Aug 9, 2021

/werft run

👍 started the job as gitpod-build-jankoehnlein-code-preview-version-4965.4

@JanKoehnlein JanKoehnlein force-pushed the jankoehnlein/code-preview-version-4965 branch from 1ca3971 to 002a1e9 Compare August 9, 2021 14:36
@JanKoehnlein JanKoehnlein force-pushed the jankoehnlein/code-preview-version-4965 branch from 002a1e9 to 503b7f2 Compare August 9, 2021 14:42
@roboquat roboquat added size/M and removed size/S labels Aug 9, 2021
@JanKoehnlein
Copy link
Contributor Author

@akosyakov I tried to use the latest commit on the gp-code branch as the "latest VS Code" commit, but then the image build fails: https://werft.gitpod-dev.com/job/gitpod-build-jankoehnlein-code-preview-version-4965.6
Is that expected? Which commit should I take instead?

@JanKoehnlein JanKoehnlein force-pushed the jankoehnlein/code-preview-version-4965 branch from 503b7f2 to 49b2b6a Compare August 9, 2021 15:19
@roboquat roboquat added size/S and removed size/M labels Aug 9, 2021
@akosyakov
Copy link
Member

akosyakov commented Aug 10, 2021

/werft run

👍 started the job as gitpod-build-jankoehnlein-code-preview-version-4965.8

@akosyakov
Copy link
Member

tooltip is off:
Screenshot 2021-08-10 at 10 52 22

@akosyakov
Copy link
Member

/unhold

@roboquat roboquat added size/M and removed size/S labels Aug 10, 2021
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

/lgtm

thanks a lot!

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: 9405196baa2314f5f4d80ea52c222b2930e384af

@JanKoehnlein
Copy link
Contributor Author

/approve

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akosyakov, JanKoehnlein

Associated issue: #4965

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit e68e76d into main Aug 10, 2021
@roboquat roboquat deleted the jankoehnlein/code-preview-version-4965 branch August 10, 2021 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[code] preview version for testing
4 participants