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

Fix-ups after the latest (several) rounds of GitHub updates #46

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ghostlandr
Copy link
Collaborator

@ghostlandr ghostlandr commented Jan 22, 2017

@Yatser

Some changes:

  • Clicking on the file name would use an anchor to put the file at the top of the screen. I at least found this annoying because it didn't use to work that way.
  • Now setting an interval to check if the HTML is injected rather than setting timeouts to call the function from within itself
  • Only injecting HTML if we're on the files page. Partly so that we can ensure the check boxes by the file name show up, but also so that the click handlers don't get set multiple times.
  • Firing up the extension on any github.com page now, simply because if you start at github.com and navigate to a PR, the extension will never load since you will not have reloaded a page.

I guess I also need to port some of this stuff to enterprise.js (done in #47)

@ghost
Copy link

ghost commented Jan 22, 2017

I wouldn't mess with enterprise.js unless you can confirm that it has changed... the reason there are two separate files is the enterprise version doesn't change as often. So unless we can somehow verify enterprise needs these changes, we shouldn't change it.

I had an enterprise demo VM but it was only good for so long so I deleted it...

@ghost
Copy link

ghost commented Jan 22, 2017

Looks good except the enterprise concern...

@ghostlandr
Copy link
Collaborator Author

Maybe I'll put the enterprise stuff in a separate PR, and someone else can take on testing that at some other time. Cause you're right, I have no idea what enterprise even looks like, haha.

Graham Holtslander added 2 commits January 22, 2017 14:27
@ghostlandr
Copy link
Collaborator Author

Got to use git revert for the first time ever!

@stevennoto
Copy link
Contributor

I think the recent merge of #53 caused some conflicts. Parts of that one addressed the HTML injection issue on the Files tab. Sorry for any inconvenience; if you want to revise my approach from that PR please feel free.

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