Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Allows branding of Che-Theia #454

Merged
merged 13 commits into from
Oct 9, 2019
Merged

Allows branding of Che-Theia #454

merged 13 commits into from
Oct 9, 2019

Conversation

vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented Sep 27, 2019

What does this PR do?

Makes it possible to brand your Theia IDE to be consistent with your product.

Product name, logo, description and helpful links were extracted to external product.json JSON file.
Default product.json has been placed in che-theia/extensions/eclipse-che-theia-plugin-ext/src/resource/ directory:

{
    "name": "Eclipse Che",
    "logo": "che-logo.svg",
    "description": "Welcome To Your Cloud Developer Workspace",
    "links": {
        "Documentation": "https://www.eclipse.org/che/docs/che-7",
        "Community chat": "https://mattermost.eclipse.org/eclipse/channels/eclipse-che"
    }
}

The following screenshots shows how Theia looks with this file.

Screenshot from 2019-10-08 13-31-24
Screenshot from 2019-10-08 13-31-29

You can customize Che Theia in two steps:

  • create custom JSON file with necessary properties and place it somewhere in your container
  • set PRODUCT_JSON environment variable with path to this JSON file

Sample of product.json file.

{
    "name": "RedHat",
    "logo": "logo.png",
    "description": "Welcome To RedHat IDE",
    "links": {
        "RedHat": "https://www.redhat.com/en",
        "RedHat Wikipedia": "https://uk.wikipedia.org/wiki/Red_Hat",
        "Eclipse Che": "https://www.eclipse.org/che/",
        "Che-Theia": "https://github.com/eclipse/che-theia"
    }
}

Result

Screenshot from 2019-10-08 13-32-09

Screenshot from 2019-10-08 13-32-13

After changing theme to white

Screenshot from 2019-10-08 13-32-52

What issues does this PR fix or reference?

eclipse-che/che#14110

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>
@vitaliy-guliy vitaliy-guliy marked this pull request as ready for review September 27, 2019 11:41
@vitaliy-guliy
Copy link
Contributor Author

@benoitf I updated Content-Security-Policy for the Welcome frame. I think we will not have any issues with enabling external resources.
If the developer wants to allow everything, he can clone our Welcome plugin, remove all the restrictions, build and use.

}

.che-welcome-command-desc {
display: flex;
}

.che-welcome-command-desc .monaco-keybinding {
Copy link
Contributor

Choose a reason for hiding this comment

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

hello, why these margins are gone ? (and for cheico logo) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Style is not used

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 didn't found it in HTML, so I removed

@benoitf
Copy link
Contributor

benoitf commented Sep 27, 2019

it's not clear to me how override is done ?
by patching the source code and modifying the json file ?
I don't see quite good benefit as you still need to patch the source code. For sure it's in a single file but you still need to rebuild the product.

I was thinking that customization could have been done through custom plug-in providing these entries as we have dynamic extensibility and plug-ins can require other plug-ins or ask them.

@vitaliy-guliy
Copy link
Contributor Author

You don't need to rebuild the plugin. You can just edit branding.json inside .theia package.
But yes, it's not very convenient as you still need to change the plugin.

@benoitf
Copy link
Contributor

benoitf commented Sep 27, 2019

with dynamic extensibility I think it's not so nice to patch every component.

It means welcome plugin will have its branding json file and then the about menu as well ?
image

@vitaliy-guliy
Copy link
Contributor Author

vitaliy-guliy commented Sep 27, 2019

I still did nothing with about dialog. But wanted to go with the same way as welcome plugin.

@benoitf
Copy link
Contributor

benoitf commented Sep 27, 2019

Thinking again on the branding:

  • we need to check vscode.env is correct: vscode.env.appName should return Eclipse Che (or Code Ready Workspaces or something else) depending of the branding
  • welcome plugin should rely on this variable
  • about plug-in should get the same stuff as well.

If branding is done at compile time, then about/extension can use a branding.json file and we can provide on @eclipse-che/plugin API a branding API which is providing some documentation link, icon, etc so any plug-in could get a centralized information.
if branding is done at runtime: then it means a branding service needs to be provided as well and a plug-in will provide upon start the different values. The tricky part is to propagate changes correctly where I have some doubts.

@vitaliy-guliy
Copy link
Contributor Author

Ok. Let's implement it step-by-step:

  • if env variable is set - use it
  • if branding service provide different values set by special plugin - use it ( let't implement it later )
  • if no, use value from branding JSON

branding.json can provide 6 fields https://github.com/eclipse/che-theia/blob/labeling/plugins/welcome-plugin/src/branding.json.d.ts
Do we need to use 6 env variables?
If yes, let's decide which names we can use

    // Title for Welcome tab
    vscode.env.welcome.title
    // Icon for Welcome tab
    vscode.env.welcome.icon

    // Logo image
    vscode.env.app.logo
    // Product name
    vscode.env.app.name

    // A short description under product name
    vscode.env.app.description
    // List of hyperlinks to be shown in Help section
    vscode.env.help.links

wdyt?

@che-bot
Copy link
Contributor

che-bot commented Sep 27, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

@benoitf
Copy link
Contributor

benoitf commented Sep 27, 2019

we can't use vscode namespace except for appName and it needs to be appName not app.name
https://github.com/microsoft/vscode/blob/master/src/vs/vscode.d.ts#L6188

all other fields need to be part of che namespace @eclipse-che/plugin

I would stick with camelCase to match VS Code

What's the difference between icon and logo ?

what is the description ?

@che-bot
Copy link
Contributor

che-bot commented Sep 27, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

@vitaliy-guliy
Copy link
Contributor Author

@benoitf I'm not sure I understand you correctly.

I see that there is Variables plugin API https://github.com/eclipse/che-theia/blob/master/extensions/eclipse-che-theia-plugin/src/che-proposed.d.ts#L61

Do we need extra service for branding?

We can just describe variables for Che Theia about dialog and for Welcome plugin.
Then any custom plugin can register that variables. Che Theia about extension and Welcome plugin can read the variables and reuse.

About dialog

For About dialog we have to provide:

  • dialog title
  • product logo
  • product name

For Welcome plugin we have to provide:

  • welcome tab title
  • welcome tab icon
  • product logo (can be reused)
  • product name (can be reused)
  • product description ( subtitle, a text under product name )
  • list of links to be shown in Help section

@che-bot
Copy link
Contributor

che-bot commented Sep 27, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

@benoitf
Copy link
Contributor

benoitf commented Sep 30, 2019

Hello,

I think we don't need dialog title (can be product title)
Welcome tab title --> not sure we need as property for now

welcome tab icon, product logo should be the same : product.logo

product name is vscode.env.appName

and we need product description + links

@che-bot
Copy link
Contributor

che-bot commented Oct 7, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>
@benoitf
Copy link
Contributor

benoitf commented Oct 8, 2019

could you provide screenshot of About dialog in light theme ?
for Eclipse Che (not red hat)

@che-bot
Copy link
Contributor

che-bot commented Oct 8, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@vitaliy-guliy
Copy link
Contributor Author

@benoitf defaults with light theme

Screenshot from 2019-10-08 14-34-18

Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>
Comment on lines 32 to 39
// Temporary solution.
// We should wait for some time until proxy is being fully initialized.
setTimeout(() => {
this.proxy.$setName(this.productInfo.name);
this.proxy.$setLogo(this.productInfo.logo);
this.proxy.$setDescription(this.productInfo.description);
this.proxy.$setLinks(this.productInfo.links);
}, 500);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the expected proper solution ? it looks something that is not working when we're using timeout

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't get it there and why we have setTimeout as it'll mostly fail.

we have async everywhere so it should either be async or deleted/reworked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main part should send values to the plugin. In our plugin system main part is initialized earlier than plugin part. So, if we send the values immediately, nobody receive it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

che.product plugin API has only variables. They have to be initialized somehow before using.
We cannot use asynchronous method as you requested to use synchronous variables.

@eclipse-che eclipse-che deleted a comment from che-bot Oct 8, 2019
@eclipse-che eclipse-che deleted a comment from che-bot Oct 8, 2019
@benoitf benoitf dismissed their stale review October 8, 2019 13:59

Cannot approve due to setTimeout but no longer request changes

Signed-off-by: Vitaliy Guliy <vgulyy@redhat.com>
@vitaliy-guliy vitaliy-guliy changed the title Allows branding of Welcome plugin Allows branding of Che-Theia Oct 8, 2019
@vitaliy-guliy
Copy link
Contributor Author

@benoitf No more timeout and setters.

@che-bot
Copy link
Contributor

che-bot commented Oct 8, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@sunix
Copy link
Contributor

sunix commented Oct 9, 2019

where should the logo be located ? next to the product.json ?

@vitaliy-guliy
Copy link
Contributor Author

@sunix
Logo can be located anyway you want. It could be absolute path, relative path to the roduct.json or http / https resource.

@musienko-maxim
Copy link

This PR has been checked locally. The new regressions was not defined.

@musienko-maxim musienko-maxim self-requested a review October 9, 2019 10:07
@vitaliy-guliy vitaliy-guliy merged commit 3cc3bd7 into master Oct 9, 2019
@vitaliy-guliy vitaliy-guliy deleted the labeling branch October 9, 2019 10:08
@sunix
Copy link
Contributor

sunix commented Oct 9, 2019

@sunix
Logo can be located anyway you want. It could be absolute path, relative path to the roduct.json or http / https resource.

Awesome ! you have to mention that in the doc

@vitaliy-guliy
Copy link
Contributor Author

@sunix sure

vinokurig pushed a commit that referenced this pull request Apr 6, 2021
Revert the previous fix for eclipse-che/che#16728, and add
a new 1.33.0a plugin to the registry with the correct
node-debug dependency specified.

Signed-off-by: Eric Williams <ericwill@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants