-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix K2 for new GitHub UI #226
Changes from 15 commits
3665c56
8a4130a
e803157
0f09fff
d2969a9
eb83283
fd96fd6
fbf2d89
8e464d5
59ccfab
6eee628
6deae98
ab4410b
1cdb513
90f1e3c
dbf1cbb
91f5531
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import $ from 'jquery'; | ||
import * as API from '../../api'; | ||
|
||
/** | ||
* This class is to be extended by each of the distinct types of webpages that the extension works on | ||
|
@@ -7,6 +8,54 @@ import $ from 'jquery'; | |
export default function () { | ||
const Page = {}; | ||
|
||
const REVIEWER_CHECKLIST_URL = 'https://raw.githubusercontent.com/Expensify/App/main/contributingGuides/REVIEWER_CHECKLIST.md'; | ||
const BUGZERO_CHECKLIST_URL = 'https://raw.githubusercontent.com/Expensify/App/main/contributingGuides/BUGZERO_CHECKLIST.md'; | ||
|
||
/** | ||
* Gets the contents of the reviewer checklist from GitHub and then posts it as a comment to the current PR | ||
* @param {Event} e | ||
* @param {'bugzero' | 'reviewer'} checklistType Type of target checklist | ||
*/ | ||
const copyReviewerChecklist = async (e, checklistType) => { | ||
const checklistUrl = checklistType === 'bugzero' ? BUGZERO_CHECKLIST_URL : REVIEWER_CHECKLIST_URL; | ||
|
||
e.preventDefault(); | ||
|
||
// Get the button element | ||
const button = e.target; | ||
|
||
// Save the original content of the button | ||
const originalContent = button.innerHTML; | ||
|
||
// Replace the button content with a loader | ||
button.innerHTML = '<div class="loader" />'; | ||
|
||
try { | ||
// Fetch the checklist contents | ||
const response = await fetch(checklistUrl); | ||
|
||
if (!response.ok) { | ||
console.error(`Failed to load contents of ${checklistUrl}: ${response.statusText}`); | ||
return; | ||
} | ||
|
||
const fileContents = await response.text(); | ||
|
||
if (!fileContents) { | ||
console.error(`Could not load contents of ${checklistUrl} for some reason`); | ||
return; | ||
} | ||
|
||
// Call the API to add the comment | ||
await API.addComment(fileContents); | ||
} catch (error) { | ||
console.error('Error fetching the checklist:', error); | ||
} finally { | ||
// Restore the original button content | ||
button.innerHTML = originalContent; | ||
} | ||
}; | ||
|
||
/** | ||
* A unique identifier for each page | ||
*/ | ||
|
@@ -47,11 +96,32 @@ export default function () { | |
Page.setup = function () {}; | ||
|
||
Page.getRepoOwner = function () { | ||
return $('.author a span').text(); | ||
return document.querySelectorAll('.AppHeader-context-item-label.Truncate-text')[0].textContent.trim(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB, I wonder if we could keep the repeatedly used selectors as constant with meaningful names of what we are selecting - For example - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For sure, I agree with that. Once we get this PR of the gate I'll spend some time to reorganize the selectors a little bit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be great. Thank you!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB ➕ I would also like to see this. Is it easy enough to do in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The repeating selectors are now removed, but I've added a bunch of comments for existing selectors to clarify what exactly they are selecting. I chose comments over variables to not have to introduce additional null checks to satisfy ESLint for selectors that turn out empty. |
||
}; | ||
|
||
Page.getRepo = function () { | ||
return $('.js-current-repository').text(); | ||
return document.querySelectorAll('.AppHeader-context-item-label.Truncate-text')[1].textContent.trim(); | ||
}; | ||
|
||
/** | ||
* Renders buttons for copying checklists in issue/PR bodies | ||
* @param {'bugzero' | 'reviewer'} checklistType Type of target checklist | ||
*/ | ||
Page.renderCopyChecklistButtons = function (checklistType) { | ||
// Look through all the comments on the page to find one that has the template for the copy/paste checklist button | ||
// eslint-disable-next-line rulesdir/prefer-underscore-method | ||
$('.markdown-body > p').each((i, el) => { | ||
const commentHtml = $(el).html(); | ||
|
||
// When the button template is found, replace it with an HTML button and then put that back into the DOM so someone can click on it | ||
if (commentHtml && commentHtml.indexOf('you can simply click: [this button]') > -1) { | ||
const newHtml = commentHtml.replace('[this button]', '<button type="button" class="btn btn-sm k2-copy-checklist">HERE</button>'); | ||
$(el).html(newHtml); | ||
|
||
// Now that the button is on the page, add a click handler to it (always remove all handlers first so that we know there will always be one handler attached) | ||
$('.k2-copy-checklist').off().on('click', e => copyReviewerChecklist(e, checklistType)); | ||
} | ||
}); | ||
}; | ||
|
||
return Page; | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these error cases we should revert the loader. So instead of logging and returning we could throw an error and have the catch and finally block handle things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the original code moved to a different function, but I changed it to:
this way the loader will always be reverted, and:
fetch()
returns something other than 200, it will throw anywayAPI.addComment()
will throw due to incorrect parametersSo all error cases are covered now.