-
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
Merged
Merged
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
3665c56
Fixed checklist copy buttons for new UI
mjasikowski 8a4130a
Fixed k2 for new github UI
mjasikowski e803157
Fixed k2 for new github UI
mjasikowski 0f09fff
Fixed k2 for new github UI
mjasikowski d2969a9
Removed leftover webpack config
mjasikowski eb83283
Updated version numbers
mjasikowski fd96fd6
Updated CHANGELOG.md
mjasikowski fbf2d89
Fixed selectors
mjasikowski 8e464d5
Fixed owner behavior
mjasikowski 59ccfab
Fixed owner behavior, cleanup + eye candy
mjasikowski 6eee628
Fixed checklist copy for issues
mjasikowski 6deae98
Fixed checklist copy for issues
mjasikowski ab4410b
Update src/js/lib/pages/github/_base.js
mjasikowski 1cdb513
Fixed panel buttons, reverted back to previous assignee behavior
mjasikowski 90f1e3c
Merge remote-tracking branch 'origin/github-new-ui-fix' into github-n…
mjasikowski dbf1cbb
Added selector comments
mjasikowski 91f5531
Fixed the copy checklist error handling
mjasikowski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ const copyReviewerChecklist = (e) => { | |
const renderCopyChecklistButton = () => { | ||
// 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 | ||
$('.js-comment-body').each((i, el) => { | ||
$('.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 | ||
|
@@ -55,54 +55,106 @@ const renderCopyChecklistButton = () => { | |
|
||
const refreshHold = function () { | ||
const prTitle = $('.js-issue-title').text(); | ||
const prAuthor = $('.pull-header-username').text(); | ||
const prAuthor = $('.gh-header-meta .author').text(); | ||
const getCurrentUser = API.getCurrentUser(); | ||
const branchName = $('.head-ref').text(); | ||
const branchName = $('.head-ref span').text(); | ||
|
||
const isNewMerge = $('div[data-testid="mergebox-partial"]').length; | ||
|
||
// Classic merge experience | ||
if (!isNewMerge) { | ||
if (prTitle.toLowerCase().indexOf('[hold') > -1 || prTitle.toLowerCase().indexOf('[wip') > -1) { | ||
$('.branch-action') // entire section | ||
.removeClass('branch-action-state-clean') | ||
.addClass('branch-action-state-dirty'); | ||
$('.merge-message button') // merge pull request button | ||
.removeClass('btn-primary') | ||
.attr('disabled', 'disabled'); | ||
// eslint-disable-next-line rulesdir/prefer-underscore-method | ||
$('.branch-action-item').last().find('.completeness-indicator') // Last section | ||
.removeClass('completeness-indicator-success') | ||
.addClass('completeness-indicator-problem') | ||
.end() | ||
.find('.status-heading') | ||
.text('This pull request has a hold on it and cannot be merged') | ||
.end() | ||
.find('.status-meta') | ||
.html('Remove the HOLD or WIP label from the title of the PR to make it mergeable') | ||
.end() | ||
.find('.octicon') | ||
.removeClass('octicon-check') | ||
.addClass('octicon-alert'); | ||
} | ||
|
||
if (!(branchName.toLowerCase() === 'master' || branchName.toLowerCase() === 'main') && !isSelfMergingAllowed() && getCurrentUser === prAuthor) { | ||
$('.branch-action') | ||
.removeClass('branch-action-state-clean') | ||
.addClass('branch-action-state-dirty'); | ||
$('.merge-message button') | ||
.removeClass('btn-primary') | ||
.attr('disabled', 'disabled'); | ||
// eslint-disable-next-line rulesdir/prefer-underscore-method | ||
$('.branch-action-item').last().find('.completeness-indicator') | ||
.removeClass('completeness-indicator-success') | ||
.addClass('completeness-indicator-problem') | ||
.end() | ||
.find('.status-heading') | ||
.text('You cannot merge your own PR.') | ||
.end() | ||
.find('.status-meta') | ||
.html('I\'m sorry Dave, I\'m afraid you can\'t merge your own PR') | ||
.end() | ||
.find('.octicon') | ||
.removeClass('octicon-check') | ||
.addClass('octicon-alert'); | ||
} | ||
return; | ||
} | ||
|
||
if (prTitle.toLowerCase().indexOf('[hold') > -1 || prTitle.toLowerCase().indexOf('[wip') > -1) { | ||
$('.branch-action') | ||
.removeClass('branch-action-state-clean') | ||
.addClass('branch-action-state-dirty'); | ||
$('.merge-message button') | ||
.removeClass('btn-primary') | ||
$('div[data-testid="mergebox-partial"] > div > div:last-of-type') | ||
.removeClass('borderColor-success-emphasis'); | ||
$('div[data-testid="mergebox-partial"] > div > div > div button').first() // merge pull request button | ||
.css({backgroundColor: 'var(--bgColor-neutral-muted)', borderColor: 'var(--bgColor-neutral-muted)'}) | ||
.attr('disabled', 'disabled'); | ||
$('div[data-testid="mergebox-partial"] > div > div button[data-component="IconButton"]').first() | ||
.css({backgroundColor: 'var(--bgColor-neutral-muted)', borderColor: 'var(--bgColor-neutral-muted)'}) | ||
.attr('disabled', 'disabled'); | ||
$('div[data-testid="mergebox-partial"] > div > div > div > div > div') | ||
.css({borderColor: 'var(--bgColor-neutral-muted)'}); | ||
$('div[data-testid="mergeability-icon-wrapper"] div').css({backgroundColor: 'var(--bgColor-neutral-emphasis)'}); | ||
// eslint-disable-next-line rulesdir/prefer-underscore-method | ||
$('.branch-action-item').last().find('.completeness-indicator') | ||
.removeClass('completeness-indicator-success') | ||
.addClass('completeness-indicator-problem') | ||
.end() | ||
.find('.status-heading') | ||
.text('This pull request has a hold on it and cannot be merged') | ||
.end() | ||
.find('.status-meta') | ||
.html('Remove the HOLD or WIP label from the title of the PR to make it mergeable') | ||
.end() | ||
.find('.octicon') | ||
.removeClass('octicon-check') | ||
.addClass('octicon-alert'); | ||
$('div[data-testid="mergebox-partial"] > div > div > section:last-of-type svg') // Last section | ||
.parent() | ||
.removeClass('bgColor-success-emphasis') | ||
.css({backgroundColor: 'var(--bgColor-neutral-emphasis)'}); | ||
$('div[data-testid="mergebox-partial"] > div > div > section:last-of-type h3') | ||
.text('This pull request has a hold on it and cannot be merged'); | ||
$('div[data-testid="mergebox-partial"] > div > div > section:last-of-type p') | ||
.html('Remove the HOLD or WIP label from the title of the PR to make it mergeable'); | ||
} | ||
|
||
if (!(branchName.toLowerCase() === 'master' || branchName.toLowerCase() === 'main') && !isSelfMergingAllowed() && getCurrentUser === prAuthor) { | ||
$('.branch-action') | ||
.removeClass('branch-action-state-clean') | ||
.addClass('branch-action-state-dirty'); | ||
$('.merge-message button') | ||
.removeClass('btn-primary') | ||
$('div[data-testid="mergebox-partial"] > div > div:last-of-type') | ||
.removeClass('borderColor-success-emphasis'); | ||
$('div[data-testid="mergebox-partial"] > div > div > div button').first() // merge pull request button | ||
.css({backgroundColor: 'var(--bgColor-neutral-muted)', borderColor: 'var(--bgColor-neutral-muted)'}) | ||
.attr('disabled', 'disabled'); | ||
$('div[data-testid="mergebox-partial"] > div > div button[data-component="IconButton"]').first() | ||
.css({backgroundColor: 'var(--bgColor-neutral-muted)', borderColor: 'var(--bgColor-neutral-muted)'}) | ||
.attr('disabled', 'disabled'); | ||
$('div[data-testid="mergebox-partial"] > div > div > div > div > div') | ||
.css({borderColor: 'var(--bgColor-neutral-muted)'}); | ||
$('div[data-testid="mergeability-icon-wrapper"] div').css({backgroundColor: 'var(--bgColor-neutral-emphasis)'}); | ||
// eslint-disable-next-line rulesdir/prefer-underscore-method | ||
$('.branch-action-item').last().find('.completeness-indicator') | ||
.removeClass('completeness-indicator-success') | ||
.addClass('completeness-indicator-problem') | ||
.end() | ||
.find('.status-heading') | ||
.text('You cannot merge your own PR.') | ||
.end() | ||
.find('.status-meta') | ||
.html('I\'m sorry Dave, I\'m afraid you can\'t merge your own PR') | ||
.end() | ||
.find('.octicon') | ||
.removeClass('octicon-check') | ||
.addClass('octicon-alert'); | ||
$('div[data-testid="mergebox-partial"] > div > div > section:last-of-type svg') // Last section | ||
.parent() | ||
.removeClass('bgColor-success-emphasis') | ||
.css({backgroundColor: 'var(--bgColor-neutral-emphasis)'}); | ||
$('div[data-testid="mergebox-partial"] > div > div > section:last-of-type h3') | ||
.text('You cannot merge your own PR'); | ||
$('div[data-testid="mergebox-partial"] > div > div > section:last-of-type p') | ||
.html('It\'s like giving yourself a high-five in public.'); | ||
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. 🤩 |
||
} | ||
}; | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
NAB, I wonder if we could keep the repeatedly used selectors as constant with meaningful names of what we are selecting - For example -
const selectorForRepoLabel = '.AppHeader-context-item-label.Truncate-text';
, so next time github changes UI, we could simply change the constant instead of going all over the code, and hopefully it should be simple to transition and works 🙏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.
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 comment
The 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 comment
The 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 comment
The 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.