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

Request and Deploy Title Copy Link #1164

Merged
merged 11 commits into from Aug 2, 2016
Merged

Request and Deploy Title Copy Link #1164

merged 11 commits into from Aug 2, 2016

Conversation

@Calvinp
Copy link
Contributor

Calvinp commented Jul 21, 2016

Adds a link next to the request ID to copy it to the clipboard.

TODO:

  • Make the link disappear on mouse off
  • Properly handle short request IDs
…IDs have a hover button that goes far to the right
const requestIdToDisplay = Utils.maybe(requestAPI, ['data', 'request', 'id']) || requestId;

return (
<div onMouseOver={() => this.onMouseOver()}>

This comment has been minimized.

Copy link
@zdhickman

zdhickman Jul 21, 2016

If this.onMouseOver is bound, you can just do onMouseOver={this.onMouseOver}

@Calvinp
Copy link
Contributor Author

Calvinp commented Jul 29, 2016

Trying to have a link pop up on hover that could be clicked to copy things wasn't working. Instead, now the requestID itself can be clicked to copy it.

An overlay trigger explains this fact to the user when they hover over the request id.

@Calvinp
Copy link
Contributor Author

Calvinp commented Jul 29, 2016

@tpetr @kwm4385 @wolfd this PR is good to be reviewed/merged.

@Calvinp Calvinp changed the title Request Title Copy Link (WIP) Request Title Copy Link Jul 29, 2016
@@ -39,13 +40,22 @@ const RequestTitle = ({requestId, requestAPI}) => {
);
}

const requestIdToDisplay = Utils.maybe(requestAPI, ['data', 'request', 'id']) || requestId;

This comment has been minimized.

Copy link
@tpetr

tpetr Aug 2, 2016

Member

why bother with the maybe() if we have the requestId variable?

This comment has been minimized.

Copy link
@Calvinp

Calvinp Aug 2, 2016

Author Contributor

Honestly, I don't know. I copied it from what was there before (see line 48 on the other side of the diff), but I can't justify it.
I'll delete it - requestId is marked as required in propTypes after all.

@Calvinp
Copy link
Contributor Author

Calvinp commented Aug 2, 2016

I also added the same ability to the Deploy Detail page. It doesn't make sense to add it to the Task Detail page because the task ID isn't displayed as the title.

@Calvinp Calvinp changed the title Request Title Copy Link Request and Deploy Title Copy Link Aug 2, 2016
@tpetr
Copy link
Member

tpetr commented Aug 2, 2016

LGTM

@Calvinp Calvinp merged commit 15298c1 into decaf Aug 2, 2016
@Calvinp Calvinp deleted the request_title_copy_link branch Aug 2, 2016
@tpetr tpetr modified the milestone: 0.10.0 Aug 18, 2016
@tpetr tpetr mentioned this pull request Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.