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 oc login quick command to help dropdown #1788

Merged
merged 1 commit into from Sep 18, 2017

Conversation

wgordon17
Copy link
Contributor

Signed-off-by: Will Gordon wgordon@redhat.com

@wgordon17 wgordon17 requested a review from jwforres June 30, 2017 22:36
@wgordon17
Copy link
Contributor Author

Screenshot:
screen shot 2017-06-30 at 18 26 59

@jwforres I can't say how often I hate having to go to the Command Line page, just to login. And the problem is compounded when using multiple clusters, as the OpenShift Online users will soon be doing when switching between Starter and Pro clusters. I think something like this would go a long way to helping ease some of that pain.

@wgordon17
Copy link
Contributor Author

@jwforres Do you think this is something that could be beneficial or belong in origin?

@spadgett
Copy link
Member

I'd rather have a normal menu item with text like "Copy Login Command" above "About." clipboard.js supports copying without showing the input.

My only concern is that we don't give a warning about the access tokens this way. We might need a modal or toast notification to warn about this after the copy.

@wgordon17
Copy link
Contributor Author

wgordon17 commented Jul 17, 2017 via email

@spadgett
Copy link
Member

Might be better under the user menu as well. That's probably where I would look as a new user.

@jwforres any objections with the proposed changes?

@wgordon17 wgordon17 force-pushed the login-shortcut branch 2 times, most recently from 6f60a06 to e22f9eb Compare July 18, 2017 21:00
@wgordon17
Copy link
Contributor Author

Updated screenshots:

Copy Login Command option
screen shot 2017-07-18 at 17 01 45

Successful copy event Token warning
screen shot 2017-07-18 at 17 01 51

@wgordon17
Copy link
Contributor Author

@spadgett Something like that ^^?

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2017
@wgordon17 wgordon17 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2017
Signed-off-by: Will Gordon <wgordon@redhat.com>
@wgordon17
Copy link
Contributor Author

@spadgett @jwforres Anything else you need from me on this?

@spadgett
Copy link
Member

[test]

@openshift-bot
Copy link

Evaluated for origin web console test up to 23d188b

@openshift-bot
Copy link

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/66/) (Base Commit: 2ecf744) (PR Branch Commit: 23d188b)

@spadgett
Copy link
Member

@wgordon17 Sorry for letting this sit for so long. I noticed that the case of the menu item "Copy Login Command" doesn't match "Log out" below.

I might change the primary text of the toast to confirm that the command was successfully copied to the clipboard, and then add the warning as the detail text below (or as a separate alert). Maybe it should be a success alert since the copy worked.

@openshift/team-ux-review Thoughts on this change? Copying the login command is pretty common, and it's a bit hard to find on the "Command Line Tools" page.

@wgordon17
Copy link
Contributor Author

@spadgett Completely makes sense! Just let me know your updates here and I can re-commit =)

@openshift/team-ux-review The other reason for this besides being hard to find, is that to get the login command requires navigating to an entirely separate page. With how common this is, it would be ideal to get the login command without any navigation necessary!

@serenamarie125
Copy link

@spadgett We should be using Headline Capitalization here, so I think that @wgordon17 did it right.

We should have "Log out" changed to "Log Out".

As for the location, yes I agree under the user menu makes more sense.
As for the actual label, I'm not sure how much it makes sense, but I'll defer to you all, since you are more familiar with the CLI than I.

We did get a comment during a review this past week that they wish we would allow the ability to disable access to this. Should we cover this in this PR as well, or should I open a separate issue @spadgett ?

NotificationsService.addNotification({
id: 'copied_to_clipboard_toast_error',
type: 'error',
message: 'Unable to copy.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Prob should say Unable to copy token or something just a touch more specific?

@spadgett
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 23d188b

@openshift-bot
Copy link

openshift-bot commented Sep 18, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/216/) (Base Commit: 164ae0a) (PR Branch Commit: 23d188b)

@openshift-bot openshift-bot merged commit b4196ee into openshift:master Sep 18, 2017
@spadgett
Copy link
Member

We did get a comment during a review this past week that they wish we would allow the ability to disable access to this.

@serenamarie125 I opened #2107 to let admins disable copy login command.

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

5 participants