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

Possible Pull Request #53

Closed
Ephellon opened this issue Jul 30, 2018 · 3 comments
Closed

Possible Pull Request #53

Ephellon opened this issue Jul 30, 2018 · 3 comments
Assignees
Labels
discussion open ended forum to keep some issues down enhancement this item makes the project better in some way forked this was implemented in the newer version(s) from a fork/merge

Comments

@Ephellon
Copy link
Collaborator

TL;DR "Should I make the PR (with 4K+ changes), or not?"

Cons

  • Firefox support is dropped
  • 4K+ changes you'd have to peruse at some point
  • All in JS instead of TS

Pros

  • Included each fix you made after fork (such as Plex login)
  • New GUI/UX
  • More sites are supported (20 so far; most from "enhancements" section)
@SpaceK33z
Copy link
Owner

Hey @Ephellon!

Wow, you made a lot of changes. Very cool. I'd love to merge the project since it wouldn't make any sense to develop separately. I will make you collaborator for this project soon.

Some things I'd like to note:

  • Could you write more descriptive commit messages in the future? Doesn't have to be long, but at least something relevant to the change.
  • There is a separate build script using Make, with this you can release a new version and automatically generate the FireFox and Chrome version. Usage: version=3.4 make release. This will also make a separate commit with the increased version number.
  • I'd like to not put the built zip and crx files in the repository itself, but use the GitHub Releases feature and upload the zip and crx files there. We can fix this after the merge.
  • Can you rename src/deps to src/sites and name the files in it index.js instead of cs.js (don't know what that means)?
  • I don't understand the use of adblockplus.txt in the repo, is this used by the code itself?
  • I use Prettier to make the code formatting consistent everywhere (and not have discussions about the formatting). At the moment Prettier isn't installed properly in the repo, so I will do that after the merge. My question to you is to install a Prettier extension in your own IDE.
  • Very minor point; I'd like to use const or let everywhere instead of var, because those are block-scoped and cause less bugs.
  • The readme will need to be adjusted to point to the Chrome and FF extension download links.
  • I can see why you've removed the manifest_firefox.json, but we'll need to find another workaround to the problem (Firefox using some keys that makes Chrome reject uploading it to the store), since I want to automate the release for both FF and Chrome.
  • As for giving you rights to upload the extension to the FF and Chrome stores, the people who make Refined GitHub have automated this whole process. Travis CI can publish it to both stores when a commit is pushed. This may be very nice for us to use too in the future.

So in short, I think the way forward is to;

  • Prepare your repository for merging (Ephellon)
    • Change references of "Web to Plex+" to "Web to Plex"
    • Probably other stuff
  • Make a PR to repository of SpaceK33z (Ephellon)
  • Merge the PR (SpaceK33z)
  • Make Ephellon collaborator (SpaceK33z)
  • Make some of the changes I mentioned above (Ephellon and SpaceK33z)
  • Thoroughly test all the changes (mostly SpaceK33z since I'd like to see all your changes)
  • Write a changelog
  • Release a new major version manually (v4)
  • Make more of the changes I mentioned above, especially automate publishing to the stores so Ephellon can release as well

How does that sound to you?

Do note that I have limited time so it could take a few weeks before we can publish v4.

@Ephellon
Copy link
Collaborator Author

Ephellon commented Aug 6, 2018

Sorry for not replying, but making notes of everything I've changed, should be done in a day or so

@Ephellon
Copy link
Collaborator Author

Ephellon commented Aug 7, 2018

Changes to "Web to Plex+" ("Web to Plex")

TODO - things I need to complete

  • remove /adblockplus.txt (won't be used)
    • this was supposed to be a custom filter list
  • install make
  • make changes to code notes

DONE - things I've completed

  • rename /deps to /sites
  • rename .../cs.* to .../index.*
    • "cs" stood for "content scripts"
  • change var to let (or const)
    • most were in/out of for loops
  • replace instances of "Web to Plex+" with "Web to Plex"

CHANGELOG - I made updates too (v2 / v4)

  • updated manifest to reflect above changes (WtP+ v2 / WtP v4)
  • /options/index.html, /options/index.js
    • added "code layout" comment
    • added "server url" option (Web to Plex | Options | Plex Settings | Advance | Server Options | Server URL)
      • this option allows the user to specify a Plex URL (e.g. localhost:32400) for faster requests and/or security concerns
      • would like to allow an array of URLs (as an access list)
        • would need a GUI update (to mimic Plex's add/remove UI/UX)
  • replaced console with terminal for issue #21
  • updated String.prototype.toCaps to not capitalize "on" in titles

Notes (to myself)

  • need to learn how to use make; as version=[version] make release to build code
    • no idea how to do that (I don't have make)
  • use Prettier to format code
    • using Brackets (found a Prettier plugin)
  • update commit messages (more descriptive)
    • duly noted
  • redo code? to make it easier to copy/paste (plugin ready)

@Ephellon Ephellon added enhancement this item makes the project better in some way discussion open ended forum to keep some issues down labels Dec 15, 2018
@Ephellon Ephellon self-assigned this Jun 28, 2019
@Ephellon Ephellon added the forked this was implemented in the newer version(s) from a fork/merge label Jun 28, 2019
This was referenced Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion open ended forum to keep some issues down enhancement this item makes the project better in some way forked this was implemented in the newer version(s) from a fork/merge
Projects
None yet
Development

No branches or pull requests

2 participants