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.
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
Fix K2 for new GitHub UI #226
Changes from 12 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
There are no files selected for viewing
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.
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.
This file was deleted.