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

CHE-1769: make workspace creation and edit screen identical. #2706

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

akurinnoy
Copy link
Contributor

@akurinnoy akurinnoy commented Oct 5, 2016

What does this PR do?

It makes workspace creation and edit screen identical.

screenshot_createflow
screenshot_editflow

What issues does this PR fix or reference?

#1769

Minor change checklist

  • New API required?
  • API updated
  • Tests provided / updated
  • Tests passed

Signed-off-by: Oleksii Kurinnyi okurinnyi@codenvy.com

@slemeur
Copy link
Contributor

slemeur commented Oct 5, 2016

Let's discuss that this afternoon @akurinnoy .

  1. I don't understand why on the first screenshot you have "Stacks" + "Runtime"
  2. I think on the "Settings" page, we need to display all the fields even in inactive (=not editable state)
  3. I'd like to add an icon for "Runtime" ;)

@codenvy-ci
Copy link

Build # 624 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/624/ to view the results.

this.newEnvironmentName = this.environmentName;
this.environment = this.workspaceConfig.environments[this.environmentName];

if (!this.environment.recipe) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add an error message and move up peas of code with machines. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is about stack's selector initialization. I will move this check into suitable place.

'use strict';
import {CheWorkspace} from '../../../../components/api/che-workspace.factory';
import {ComposeEnvironmentManager} from '../../../../components/api/environment/compose-environment-manager';
import {CheEnvironmentRegistry} from '../../../../components/api/environment/che-environment-registry.factory';
Copy link
Contributor

@olexii4 olexii4 Oct 5, 2016

Choose a reason for hiding this comment

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

I didn't see a benefit of using these types, because we add many path's dependencies in many places. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@olexii4 I don't see the issue of typing with correct type ?

Copy link
Contributor

@olexii4 olexii4 Oct 5, 2016

Choose a reason for hiding this comment

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

@benoitf I guess we could try to add a che namespace. I think about it. Not in this issue, but I want try to do it. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@olexii4 I'm ok with both solutions. namespace will take some time to be added while specific import is immediate.

@ashumilova ashumilova added the status/in-progress This issue has been taken by an engineer and is under active development. label Oct 6, 2016
@codenvy-ci
Copy link

Build # 647 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/647/ to view the results.

@codenvy-ci
Copy link

Build # 650 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/650/ to view the results.

@codenvy-ci
Copy link

Build # 703 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/703/ to view the results.

@codenvy-ci
Copy link

Build # 718 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/718/ to view the results.

@akurinnoy akurinnoy changed the title [WIP] CHE-1769: make workspace creation and edit screen identical. CHE-1769: make workspace creation and edit screen identical. Oct 17, 2016
@slemeur
Copy link
Contributor

slemeur commented Oct 17, 2016

Ok for me.

machinesViewStatus: '=',
workspaceConfig: '=',
environmentOnChange: '&'
};
Copy link
Contributor

Choose a reason for hiding this comment

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

you may add the type for these variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

this.workspaceKey = this.namespace + ":" + this.workspaceName;
this.editMode = false;
this.showApplyMessage = false;
this.Tab = Tab;
Copy link
Contributor

Choose a reason for hiding this comment

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

we have uppercase variable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just typo, fixed

@codenvy-ci
Copy link

Build # 725 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/725/ to view the results.

Signed-off-by: Oleksii Kurinnyi <okurinnyi@codenvy.com>
@akurinnoy akurinnoy merged commit a9bdb7a into master Oct 18, 2016
@akurinnoy akurinnoy added this to the 5.0.0-M6 milestone Oct 18, 2016
@akurinnoy akurinnoy removed the status/in-progress This issue has been taken by an engineer and is under active development. label Oct 18, 2016
@codenvy-ci
Copy link

Build # 727 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/727/ to view the results.

@akurinnoy akurinnoy deleted the CHE-1769 branch November 1, 2016 07:54
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
CHE-1769: make workspace creation and edit screen identical.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants