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

Cleaned up the code and made the toolbar button intelligently enable/disable #11

Merged
merged 3 commits into from Oct 15, 2016

Conversation

kabiroberai
Copy link
Contributor

The title (and commits) say it all. I've removed a lot of things that I didn't think were needed, but if they are supposed to be for future features, feel free to select what to merge.

@kabiroberai
Copy link
Contributor Author

There also seem to be a few bugs with the toolbar enabling/disabling thing, and I think that activate isn't always being called on tab change. This may be worth looking into.

@kabiroberai
Copy link
Contributor Author

Also, this doesn't seem to be working with Netflix, since the video tag there seems to be loaded dynamically (it may require a Mutation Observer).

@arnoappenzeller
Copy link
Owner

I'm going to look into it in the next few days but already thanks in advance - sounds fantastic.

Netflix worked for me a few weeks ago but maybe they did some changes in their code

@arnoappenzeller arnoappenzeller merged commit 0567e94 into arnoappenzeller:master Oct 15, 2016
@arnoappenzeller
Copy link
Owner

arnoappenzeller commented Oct 15, 2016

The cleanup is very welcome and you finally made the activation feature work.

Didn't think about the Mutation Observer - this was where I was stuck in the first place so I'm merging this!

Will also add the additions for the AppStore version soon

Thanks so much!

@kabiroberai
Copy link
Contributor Author

@arnoappenzeller no problem 👍 Also, I've made another commit in my fork that removes both the local reference to stateManager (now there's no self capturing in validateToolbarItem's getActivePage), and the firstCheck variable in script.js (instead, previousResult is now null during the first loop). You could manually do those here or I can make another PR if you want.

@arnoappenzeller
Copy link
Owner

Feel free to add another PR :-)

Otherwise I'm currently working on a new feature (adds buttons directly to popular players) which I will push later or next weekish then I can include those changes

@arnoappenzeller
Copy link
Owner

I added some of your changes in 2d783e0

I didn't add the changes to the js because after a while the button didn't activate/deactivate properly in my tests.

@kabiroberai
Copy link
Contributor Author

@arnoappenzeller I figured out the issue (it happens even with the old logic), so expect a PR soon :)

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.

None yet

2 participants