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

feat: add support for setting global speed limits #406

Merged

Conversation

giacomocerquone
Copy link
Contributor

@giacomocerquone giacomocerquone commented Apr 16, 2022

Allowing the control of global speed limits [feat]

First of all, great project and a very clean code. Never used Vue before and I implemented this in less than an hour.
I tried to maximize the reuse in this pr that, as stated in the title, allows users to set global speed limits (both upload and download).
It reuses the SpeedLimitModal component that appears after clicking over the download and upload card speeds in the right drawer of the webpage.

Here is a screencast: https://watch.screencastify.com/v/a09SoF3H2Bt9flO7N1UI
(After settings the global limits you might observe some spikes in the reading of the speed data but the limits are correctly applied)

Closes #339
Enjoy 😃

UPDATE:

In a future update, we can also consider adding this stuff inside the settings

PR Checklist

  • I've started from master
  • I've only committed changes related to this PR
  • All Unit tests pass
  • I've removed all commented code
  • I've removed all unneeded console.log statements

@WDaan WDaan mentioned this pull request Apr 16, 2022
@WDaan
Copy link
Collaborator

WDaan commented Apr 16, 2022

Hi!

PR looks great! I personally find it really intuitive to click the cards above the grap to show the modal, very clever.

As you mentioned, I do value easy/clean/readable code... so I branched your PR with some small changes to make it more readable in the future.

Either we merge that one or update this one or idk, whatever works for you 🤷‍♂️

Thank you for contributing 😄

@giacomocerquone
Copy link
Contributor Author

@WDaan thanks, I can definitely bring them here and merge this! Agree with the enhancements you've done, I wanted to touch as little code as possible due to my lack of vue knowledge 😄

@giacomocerquone
Copy link
Contributor Author

giacomocerquone commented Apr 16, 2022

Actually no, my commits would still stay in the history even merging yours. So go ahead and merge your pr (just don't squash)! Thanks, I'm gonna close this

@WDaan
Copy link
Collaborator

WDaan commented Apr 16, 2022

Hmmm I don't think that will work. The auto generated changelogs will pick up the 2 feat: and the perf: commit as separate changes. And squashing won't show your contribution.

Maybe you should merge it in here so I can squash-merge this one? I do apologise for the inconvenience.

@giacomocerquone
Copy link
Contributor Author

Yes if you do squash the commits my commits won't be shown. Let me merge them back here then 🥺😃

@WDaan WDaan changed the title feat: global_speed_limits_control feat: add support for setting global speed limits Apr 16, 2022
@WDaan
Copy link
Collaborator

WDaan commented Apr 16, 2022

I think we're good now?

Thanks for the fixes and the contribution 😄

@giacomocerquone
Copy link
Contributor Author

My own pleasure.
Tested it with the refactored code and it's all good 😄 I think you can go.

@WDaan WDaan merged commit 23fee41 into VueTorrent:master Apr 16, 2022
@giacomocerquone giacomocerquone deleted the gc-feat/global_speed_limits_control branch April 16, 2022 11:13
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.

Add settings for global speed limits
2 participants