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

CHE-11443 Implement the containers plugin #13

Merged
merged 3 commits into from
Jan 11, 2019
Merged

CHE-11443 Implement the containers plugin #13

merged 3 commits into from
Jan 11, 2019

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented Jan 9, 2019

Implement the containers plugin

fixes eclipse-che/che#11443

screenshot from 2019-01-09 11-43-19


}

export class TestDataProvider implements theia.TreeDataProvider<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@olexii4 why it's called TestDataProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Just forgot to rename. I will fix it.

@olexii4 olexii4 changed the title CHE-11443 Implement the containers plugin [WIP] CHE-11443 Implement the containers plugin Jan 9, 2019

export class TestDataProvider implements theia.TreeDataProvider<string> {

private onDidChangeTreeDataEmitter: theia.EventEmitter<any> = new theia.EventEmitter<any>();
Copy link
Contributor

Choose a reason for hiding this comment

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

EventEmitter has dispose method, I think it could be utilized: https://github.com/theia-ide/theia/blob/master/packages/plugin/src/theia.d.ts#L1722.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

export class TestDataProvider implements theia.TreeDataProvider<string> {

private onDidChangeTreeDataEmitter: theia.EventEmitter<any> = new theia.EventEmitter<any>();
readonly onDidChangeTreeData: theia.Event<any> = this.onDidChangeTreeDataEmitter.event;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe void should be better instead of any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Oleksii Orel <oorel@redhat.com>
@olexii4
Copy link
Contributor Author

olexii4 commented Jan 10, 2019

screenshot from 2019-01-10 16-20-22

@olexii4 olexii4 changed the title [WIP] CHE-11443 Implement the containers plugin CHE-11443 Implement the containers plugin Jan 10, 2019
return uniqueId;
}

dispose(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could return Disposible here and simplify code upper.

Copy link
Contributor Author

@olexii4 olexii4 Jan 10, 2019

Choose a reason for hiding this comment

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

I don't think so

@vitaliy-guliy
Copy link
Contributor

UI improvements:

  • rename terminal node to New terminal
  • add icon and Server: prefix to the server node

WDYT?

Signed-off-by: Oleksii Orel <oorel@redhat.com>
@olexii4
Copy link
Contributor Author

olexii4 commented Jan 10, 2019

@olexii4
Copy link
Contributor Author

olexii4 commented Jan 10, 2019

screenshot from 2019-01-10 20-11-48

Signed-off-by: Oleksii Orel <oorel@redhat.com>
@olexii4
Copy link
Contributor Author

olexii4 commented Jan 11, 2019

screenshot from 2019-01-11 09-54-49

@slemeur
Copy link

slemeur commented Jan 11, 2019

@olexii4 : Thanks for adding the screenshots to the PR.

Quick feedbacks:

  • When a developer is in the context of building a Theia Plugin or doing Che in Che, I understand that you want to see the theia container. But in a general use case, when a developer is building an application (a NodeJS one for example), I don't think it's necessary to display the tooling containers by default. I would see an option to display/hide those tooling containers. (That can be added later, as there is another issue for that, related to tasks Apply ability filter user containers from among others che#11804)
  • Is there something happening when I'm doing a right click on one of the container? For example, execute a task in that container directly?

@olexii4
Copy link
Contributor Author

olexii4 commented Jan 11, 2019

@slemeur

  1. Great idea. I will add it after Apply ability filter user containers from among others che#11804.
  2. Nothing happens when you are doing a right click on one of the treeView items. Implemented API for plugins doesn't support it.

@slemeur
Copy link

slemeur commented Jan 11, 2019

On 2. Is it an API gap or is it something that just does not exist?

@olexii4
Copy link
Contributor Author

olexii4 commented Jan 11, 2019

@slemeur
If I am not mistaken, it is doesn't supported in plugin API at all

@l0rd
Copy link
Contributor

l0rd commented Jan 11, 2019

This is really cool! One comment: we should replace servers with endpoints or services.

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.

Theia Machine Plugin: add missing API and implement the plugin
7 participants