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

Newznab module added #20

Closed
wants to merge 4 commits into from
Closed

Newznab module added #20

wants to merge 4 commits into from

Conversation

Jeroenve
Copy link
Contributor

Module shows the x most recent items from Series and Movies.

Additional features/options will be added.

Some ideas:

  • Show most recent added Series
  • Show most recent added Movies
  • Search function

@Jeroenve Jeroenve mentioned this pull request Dec 29, 2011
@Jeroenve
Copy link
Contributor Author

Got it also working with other newznab providers (like nzb.su).

Resulting in small problem for Spotweb, fixed by implementing rewritting
RewriteRule ^details/?(.*)$ /spotweb/?page=getspot&messageid=$1 [L,R]

RewriteRule ^details/?(.*)$ ?page=getspot&messageid=$1 [L,QSA] resulted in a white page with correct source.

@gugahoi
Copy link
Collaborator

gugahoi commented Dec 30, 2011

I have been reviewing this and it is almost ready to be merged into "experimental". I do have a couple of tips to make your code more likely to get merged:

1st: it is always good practice to modify code on a separate branch in your repo and not in master

2nd: I'm not so sure how to explain but you might have done a chmod or something as it seems there are tons of files that have been modified including just about all images. This makes reviewing the code much harder as I do not know which ones were actually modified by you and it is way too much trouble to open up every single file (85 according to git) and check. I will try a merge anyway onto experimental but it will be less likely to make it to master. Specially as it will require quite some time to review this thoroughly.

3rd: it would be nice if you could convert your JS functions to start using .on instead of .delegate. I know most of the functions actually use .live but we only found out about .on recently. If you want to have a look on how it works, just see the file on the experimental branch as I have converted them this time. --> refer to 4ea5da2#diff-7

I also wanted to say I do not have SpotWeb installed and therefore am not sure the functions within this module work. I have checked that no errors are thrown and am counting on someone else (possibly @Mar2zz) checking this for me.

@gugahoi
Copy link
Collaborator

gugahoi commented Dec 30, 2011

Okay, module merged. Please test it out and give me some feedback.

@Jeroenve
Copy link
Contributor Author

This is my first real project on github. So from now on I will use a seperate branch for it.

@gugahoi
Copy link
Collaborator

gugahoi commented Dec 30, 2011

That's ok, that's why I said it was just a tip. I also found a couple of bugs with the styling, could you post a pic of how the module should look?

@Jeroenve
Copy link
Contributor Author

Preview how it looks:
Newznab module

I copied your fixes in style sheet. Try to finish today the search module and commit afterwards with the new styles.

@gugahoi
Copy link
Collaborator

gugahoi commented Dec 30, 2011

Not sure wether you know but if you commit to your master the modifications will be added here.

@Jeroenve
Copy link
Contributor Author

I saw it yesterday that they were automatically added. However I have switched to a new branch ;).

So I have to merge that branch to my master to have it added?

@gugahoi
Copy link
Collaborator

gugahoi commented Dec 30, 2011

you can but now that you have switched just make a pull request from that one and keep ur master up to date with this repo's. Your call.

@Jeroenve
Copy link
Contributor Author

Aparently new commits on the newznab branch are still added. Strange

@gugahoi
Copy link
Collaborator

gugahoi commented Dec 30, 2011

That is just a reference. They are not added in the pull request yet.

@Jeroenve
Copy link
Contributor Author

Continuation can be found in #26

@gugahoi gugahoi mentioned this pull request Dec 30, 2011
@Jeroenve Jeroenve closed this Dec 30, 2011
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