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

Add to project options using the ordering overlay component #1382

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

jwforres
Copy link
Member

@jwforres jwforres commented Mar 25, 2017

Prototype of what it looks like embedding our existing deploy image form into the overlay ordering panel.

2017-03-25 17 01 25

This is a bit clunky to interact with until we can actually split these forms into wizards.

cc @serenamarie125 @spadgett

@smarterclayton
Copy link
Contributor

Crikey - that scroll is going to make me stab someone. The moment I hit "want more details" I'm going to want to use the rest of the real estate. How do we turn "in-context" into "usable with rich options" for end users?

We should vet for each of these flows the expected depth of interaction (successive choices made). For a lot of templates / builders 1-3 choices is def reasonable. But the moment I want to go deep, I'm going to get angry if I can't use my screen real estate.

Seems like we need a way to turn the restricted in-context into unrestricted nav in a compelling way. Is that on the design radar?

@jwforres
Copy link
Member Author

Updated to use the full height version of the overlay, stuff still to do:

  • overlay-panel-body overflow causing unnecessary horizontal scroll with forms that use bootstrap row
  • what to do with the Next Steps page, navigating there loses the context which defeats the purpose of doing this in the overlay
  • same question if i do import yaml and it needs to go to the template processing page
  • if import only creates / updates a single resource its using AlertMessageService to show the success message, this only gets used by the overview when it loads, so the message is lost
  • import sometimes not closing on success, its trying to navigate to the overview which is a no-op if you are already on the overview
  • if I use these overlays on other pages, should they navigate to the overview on success? depends on what im doing maybe as to what i might expect?

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2017
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2017
@jwforres jwforres force-pushed the add-project-overlay branch 2 times, most recently from eba799a to 3df41d7 Compare May 26, 2017 20:38
@jwforres
Copy link
Member Author

We are triggering a number of things about template processing from the isDialog flag, the interesting thing is in the context of the Import YAML dialog we have significantly more height in order to account for the ACE editor on the previous step. So with our default templates you end up in a situation like this one:

process template from import dialog

For reference this is the previous step:

import-yaml-dialog

I can:

  1. Reduce the height of the ACE editor when its in a dialog... but I kind of like the height its at now. Much shorter would make it hard to see the context of the object you are editing if you want to make changes. There is whitespace beneath the editor we could eliminate, currently it only is helpful if there are syntax errors in your JSON/YAML because the validation error alert is pretty tall. That will help, but not eliminate the problem.

  2. Not do the simple vs advanced modes in this context. Which would be a little strange that it behaves differently than template processing from the catalog.

  3. Leave as is in the screenshot. Which looks a bit silly.

@jwforres
Copy link
Member Author

realized its missing the description for the template and other info that we show in the left column in the catalog, that should help fill out that page.

@ncameronbritt
Copy link

ncameronbritt commented Jun 1, 2017

@spadgett and I were talking about removing the "manage your app" and "command line tools" stuff from the results step of the wizard.

Given the space constraints of the wizard, I think it makes sense to focus more on the things directly related to what you have just created. The command line tools stuff isn't specifically about what you've just created, but is more general to the project and I worry that having it in the results step could distract from other more pertinent information.

Sam's thought, which I agree with, is that the landing page is probably the place to advertise the command line tools, in that right panel. Within the project, the command line info is already in that info menu, which makes sense to me.

@openshift/team-ux-review

@spadgett spadgett changed the title [WIP] Add to project options using the ordering overlay componen [WIP] Add to project options using the ordering overlay component Jun 5, 2017
failure: "Failed to deploy image " + $scope.app.name + " to project " + $scope.project + "."
started: "Deploying image " + $scope.app.name + " to project " + $scope.project.metadata.name + ".",
success: "Deployed image " + $scope.app.name + " to project " + $scope.project.metadata.name + ".",
failure: "Failed to deploy image " + $scope.app.name + " to project " + $scope.project.metadata.name + "."
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is new, but in template processing we use the display name of the project. Probably should be consistent.

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/processTemplate.js#L81

templateUrl: 'views/directives/deploy-image-dialog.html'
});

function DeployImageDialog($scope, DataService) {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably use an IIFE to avoid the global (#1516, #1637) here and in the other components below

}
});

ctrl.currentStep = "Image";
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in $onInit?

failure: "Failed to create some resources in project " + $scope.projectName
started: "Creating resources in project " + $scope.project.metadata.name,
success: "Creating resources in project " + $scope.project.metadata.name,
failure: "Failed to create some resources in project " + $scope.project.metadata.name
Copy link
Member

Choose a reason for hiding this comment

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

Not a new problem, but same comment here on display name (here and just below)

};

function getIconClass() {
var icon = _.get(ctrl, 'template.metadata.annotations.iconClass', 'fa fa-cubes');
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should use the iconClass filter, which handles template icons. But it defaults to fa-clone for templates. This makes me wonder if we shouldn't use fa-clone as our default in the catalog.

I'm OK with this as-is, but we might open an issue to decide what to do since we can show different default icons for the new experience vs. old.

https://github.com/openshift/origin-web-console/blob/master/app/scripts/filters/resources.js#L53

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is consistent with the catalog and this dialog, for now, doesn't show up in the old experience. I'll open an issue.

@@ -0,0 +1,95 @@
<overlay-panel show-panel="$ctrl.visible" show-close="true" handle-close="$ctrl.close" ng-if="$ctrl.project">
<!-- TODO what to do with alerts? -->
Copy link
Member

Choose a reason for hiding this comment

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

TODO still needed?

<li><a ng-href="project/{{projectName}}/create?tab=deployImage">Deploy Image</a></li>
<li ng-if-end><a ng-href="project/{{projectName}}/create?tab=fromFile">Import YAML / JSON</a></li>

<li ng-if-start="catalogLandingPageEnabled"><a href="/">Browse Catalog</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I thought it should be href="./" but it must be working?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm.. weird yeah its working fine as is

</a>
<ul class="uib-dropdown-menu dropdown-menu dropdown-menu-right">
Copy link
Member

Choose a reason for hiding this comment

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

Probably should use <ul role="menu"> and each <li role="menuitem">

@@ -38,3 +47,6 @@
</div> <!-- /navbar-project-menu -->
<navbar-utility class="hidden-xs"></navbar-utility>
</nav>
<!-- clean up ordering.* -->
Copy link
Member

Choose a reason for hiding this comment

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

Not clear on what this means. Is it a TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

at one point it was a todo but i think i changed my mind, will remove comment

if (_.isFunction(ctrl.onContinue)) {
ctrl.onContinue();
}
Navigate.toProjectOverview(ctrl.projectName);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the onContinue handler since we're always navigating away here?

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2017
@jwforres
Copy link
Member Author

jwforres commented Jun 8, 2017

updated for the review feedback but i'm seeing a CSS issue at mobile now with my header, probably something related to all the flex changes and merge conflicts i had during rebase

@jwforres
Copy link
Member Author

jwforres commented Jun 8, 2017

fixed the bad visual issues at mobile, there is one thing I want to have @sg00dwin look at with me tmrw afternoon

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2017
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple small questions on the on-continue callback

NextSteps
],
bindings: {
project: '<',
projectName: '<',
loginBaseUrl: '<',
fromSampleRepo: '<',
createdBuildConfig: '<'
createdBuildConfig: '<',
onContinue: '<'
Copy link
Member

Choose a reason for hiding this comment

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

Handle the callback like onDialogClosed below?

<next-steps project="$ctrl.selectedProject"
project-name="$ctrl.selectedProject.metadata.name"
login-base-url="$ctrl.loginBaseUrl"
on-continue="$ctrl.close">
Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm still unclear why this is necessary since on-continue always navigates to the overview anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are already on the overview then navigating to the overview does nothing. Angular treats it like a no-op, so the overlay never closes.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2017
@jwforres jwforres changed the title [WIP] Add to project options using the ordering overlay component Add to project options using the ordering overlay component Jun 13, 2017
@jwforres
Copy link
Member Author

@spadgett it looked like you hadn't yet implemented toast notifications for deployImage, i'd rather do that separate from this PR I think

otherwise I think this PR is ready

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2017
@spadgett
Copy link
Member

[merge]

@jwforres
Copy link
Member Author

updated tests [test]

@openshift-bot
Copy link

Evaluated for origin web console test up to d1abdf5

@openshift-bot
Copy link

Evaluated for origin web console merge up to d1abdf5

@jwforres
Copy link
Member Author

integration tests passed [merge]

@openshift-bot
Copy link

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1516/) (Base Commit: 54262a2)

@openshift-bot
Copy link

openshift-bot commented Jun 13, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1526/) (Base Commit: 96fab4d)

@openshift-bot openshift-bot merged commit 871e99a into openshift:master Jun 13, 2017
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