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

Provide a way to manage GH issue owner #166

Merged
merged 7 commits into from
Oct 17, 2023
Merged

Provide a way to manage GH issue owner #166

merged 7 commits into from
Oct 17, 2023

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Oct 16, 2023

This PR adds a small UI to the assignees section of a GH issue to manage the owner of an issue. It will only appear when there are multiple assignees.

image

It also surfaces the owner information in the K2 dashboard so you can easily see the issues you are the owner of. It puts them at the top of the list and puts the yellow star next to them.

image

@tgolen tgolen self-assigned this Oct 16, 2023
@@ -10,7 +10,12 @@ let octokit;
*/
function getOctokit() {
if (!octokit) {
octokit = new Octokit({auth: Preferences.getGitHubToken()});
/* eslint-disable-next-line no-console */
console.log('authenticate with auth token', Preferences.getGitHubToken());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was running into a lot of issues when testing this where I kept hitting a secondary rate limit. I added this log to hopefully help trouble-shoot if it happens in production.

rafecolton
rafecolton previously approved these changes Oct 16, 2023
Copy link
Member

@rafecolton rafecolton left a comment

Choose a reason for hiding this comment

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

It was bugging me a little that the buttons weren't aligned with the rest of the pannel...

...so I did a little tinkering and here's a code change that will fix it:

diff --git a/src/css/content.scss b/src/css/content.scss
index f381e0d7..c2fbb01d 100644
--- a/src/css/content.scss
+++ b/src/css/content.scss
@@ -545,9 +545,17 @@ $color-dark-yellow: #DAA520;
   top: 60px;
 }
 
-.js-issue-assignees .k2-button {
-  position: absolute;
-  left: 106%;
+.js-issue-assignees {
+  .js-hovercard-left {
+    position: relative;
+  }
+
+  .k2-button {
+    right: 0;
+    padding: 0;
+    max-height: 100%;
+    position: absolute;
+  }
 }
 
 .alert {

Not going to block on it, since it works great

const ghDescription = response.data.body;
const newDescription = `${ghDescription}

Current Issue Owner: @${owner}`;
Copy link
Member

Choose a reason for hiding this comment

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

NAB Should we put this inside a <details> block so it's more obvious that editing it could have unintended side-effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked about that in the Slack thread and no one seemed to interested in it. I don't mind putting it in one though if you think it would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I should have specified 🤦‍♂️ I think the main value in using a details block is that it's collapsable if there's extra text telling the user not to edit the value. Just putting it inside the details block with no other info doesn't seem that useful. I realize though that this suggestion is solving a problem we don't really have, so that's my bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh got it, I missed this comment before merging. I actually think it's fine if someone chooses to manually edit it to change the person. It could be that not everyone is using the k2 extension.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 16, 2023

Updated to move button and put code in <details> tags

@tgolen tgolen merged commit be4b709 into main Oct 17, 2023
3 checks passed
@tgolen tgolen deleted the tgolen-issue-owner branch October 17, 2023 16:14
@tgolen
Copy link
Contributor Author

tgolen commented Oct 17, 2023

I'm going to merge this, but I'll hold off deploying this until the web-e stuff is in place.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 17, 2023

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.

2 participants