-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Save watched episodes #14
Comments
Hey @AndreasFurster, thank you for the feedback. The |
Really, For those that thumbed it down, you know it would help more if you actually said why! Otherwise you are just being nonconstructive!I would like to start by saying, love the site. It has been very helpful and is the best list I have found out there by far. There is a limitation to the saving since it uses cookies and there is a max on the size it can only store about 160 for you, I note this in the code in detail, but best to just check the ones you are currently working on, since the other part is not going to be showing on your screen anyway. The only workaround for this would be to have some sort of server side storage database, like MySQL. Even then, this code would still be viable using a AJAX request method added to the code. Note, make sure if you use tampermonkey you have all the matches possible, like this: |
Hi @ajsmith21 I did not downvote you're script, but I also did not start using it. First I would like to say that it is great you are trying to contribute! I could have done it myself then, but I did not took the time to do it. You did. You definitely deserve feedback. I think there are a couple of reasons why your contribution is down-voted. For starters, it is an external script to run in things like Tampermonkey. This adds a whole other requirement for using the arrowverse website. Even if the script is added to the website directly, it is still an external script. The script does stuff like disabling and re-enabling event handlers (line 53 and 230). This is completely unnecessary if the code was directly integrated into the website. Then the script... The code is ok. You know how jQuery works, and you've added comments. It's the logic that is extremely over-complicated. Think about it... Wat are actually the requirements of this feature? If I had to create a summary, I would say something like this: "Add an checkbox to each table row, and sync it's state in localStorage/cookies". Do we really need 260 lines of code for that? If we do, we are probably doing things wrong. One other example; at line 219 you say "Now this part is just stolen from you code and...". You cannot do that. Because if the code is changed in the arrowverse project, your code isn't changed with it. If you want, and @AceFire6 is ok with it, you can fork the project, and we could implement this feature into the project the right way. |
Oh, I know you did not downvote it. Some people just did without feedback, which is what would have been helpful. The use of tampermonkey was just a suggestion, if someone wanted to implement it themselves before a feature like this was added to the site. I will agree, there is probably a much shorter way to implement this. The code is far from refined. I am trying to understand your comment on line 219, I don't really understand what you are trying to say about that, can you please elaborate? As for the fork, I have to be honest, I am still trying to get used to how github works (actually all online code repositories for that matter). And again, thank you very much for the feedback! This is actually the first time I have written any code that has made use of cookies, I have never had reason to use them. I have also always had a back end database running on all my projects I have ever worked on. |
I'm glad you received it well 😄!
It's not really about how long or how short, just about how complicated it is. And a lot of it (if not all) will be solved by integrating it into the project.
You should try to prevent code duplication. Especially in an external script. If for example we want to save each wiki open to Google Analytics in the feature, the current script will override that action. This because it removes the event handler and creates a complete new one. This is not a great example and I don't see more cases in the script, but I wanted to mention it as feedback.
I highly recommend this series from The Coding Train. |
@AndreasFurster, I see why I removed and re-added that event handler, It was due to the fact that the event handler that did exists applied to the entire row when clicked on would open a new tab that went to the wiki. So I needed to replace it to only do that when clicking on the other columns of the row not the check mark portion. I agree this is not the best practice but in having no access to the base code I needed a way to control this. You are right tho, this is part of my code that would definitely need to be changed if it was to be used, along with the original event handler. Also, thank you so much for the Training Series, I will watch this as soon as I can! :D |
Just wanted to bump and see if this is still in your roadmap? I'm trying to avoid making my own version of this, in a different language I'd contribute to add it but I'm not savvy enough with Python to meaningfully add in the feature. |
Any update on this one? |
At first; thanks for building this website!
It would be awesome if you could save the episodes you've watched. Just with an selectbox before each episode. It would already be good if it's stored in LocalStorage. This should be easy to build. Also storing applied filters would be very useful.
The text was updated successfully, but these errors were encountered: