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

Don't select a default project if there is more than one project #387

Merged
merged 1 commit into from Aug 21, 2017

Conversation

dtaylor113
Copy link
Contributor

@dtaylor113 dtaylor113 commented Aug 18, 2017

The following changes have been made to the <select-project> component:

  • Accepts a list of projects, so another projects retrieval is not needed (performant)
  • Callback method when project selected.
  • Does not select a project by default, unless there is only one project
  • required asterisk, error msg if user clicks on dropdown, but doesn't select a project
  • Sorts projects by displayName

No Project Selected
image

User clicks on then off dropdown, and doesn't select a project:
image

Project Selected
image

Fixes: openshift/origin-web-console#1833

@spadgett
Copy link
Member

We should have class="required" on the label so it gets the asterisk.

We shouldn't show the red error text when the page first loads. Only if the user focuses the project select, but doesn't select anything.

@dtaylor113
Copy link
Contributor Author

@spadgett added required and error text when no project selected after losing focus.

@serenamarie125
Copy link

@spadgett curious on "Sorts projects by displayName" . Should we enhance this to sort primary by last accessed by the user & secondary by displayName?

@dtaylor113 also curious if this is ui-select if we are including the search capability here?

@dtaylor113
Copy link
Contributor Author

@dtaylor113 also curious if this is ui-select if we are including the search capability here?

Yup :-)
image

@spadgett
Copy link
Member

@spadgett curious on "Sorts projects by displayName" . Should we enhance this to sort primary by last accessed by the user & secondary by displayName?

Yeah, I think so. It's also possible to group items in ui-select, so we could have "Recent" and "All" groups if we want.

@spadgett
Copy link
Member

removed unused name-taken attribute

@dtaylor113 name-taken is being used

https://github.com/openshift/origin-web-console/search?utf8=%E2%9C%93&q=name-taken&type=

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.

We should add back name-taken and make sure it's being used properly by callers.

@@ -45,19 +43,11 @@ export class SelectProjectController implements angular.IController {
});
}

public $onChanges(onChangesObj: angular.IOnChangesObject) {
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 handle changes to the passed-in projects.

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

<div class="form-group" ng-class="{'has-error' : $ctrl.forms.selectProjectForm.selectProject.$error.cannotAddToProject ||
($ctrl.forms.selectProjectForm.selectProject.$touched &&
$ctrl.forms.selectProjectForm.selectProject.$invalid)}">
<label class="control-label required" for="project">Add to Project</label>
Copy link
Member

Choose a reason for hiding this comment

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

There's for="project", but I don't see an id="project" anywhere. Not new, but we should fix.

If we use an ID here, let's pick a more specific name that is less likely to conflict with another ID that happens to be on the page. It might be easiest just to remove that for now because I'm not sure how to set the ID of the input with ui-select.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed for="project"

@dtaylor113
Copy link
Contributor Author

@dtaylor113 name-taken is being used

I see that the HTML is using projectNameTaken, but I don't see that variable being used/set in any of the controllers. I think it needs to be removed from the HTML.

@spadgett
Copy link
Member

name-taken was replaced by osc-unique and wasn't being used anywhere

No, it can't be replaced by osc-unique. Normal users can't see all projects in the cluster, so you don't have a complete list of project names. We still need to handle errors from the server creating the project because the name is already in use by someone else.

We need to add name-taken back and make sure it's used properly everywhere. It's a bug if it was removed from any of the controllers.

@spadgett
Copy link
Member

To be clear, we should still use osc-unique so we can warn users earlier when we know a project name already exists. But we also have to handle the case where we only find out later when we try to create the project.

@dtaylor113
Copy link
Contributor Author

We need to add name-taken back and make sure it's used properly everywhere. It's a bug if it was removed from any of the controllers.

Hi @spadgett, ok added back name-taken and filed this issue: openshift/origin-web-console#1955

@spadgett
Copy link
Member

Thanks @dtaylor113 LGTM

@spadgett spadgett merged commit 7d222f5 into openshift:master Aug 21, 2017
@dtaylor113 dtaylor113 deleted the fix-select-proj-defaults branch October 19, 2017 18:31
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.

None yet

4 participants