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

Added pet interaction, Dynamic throwing! #307

Merged
merged 10 commits into from
Nov 20, 2022

Conversation

Luke-G-Cordova
Copy link
Contributor

@Luke-G-Cordova Luke-G-Cordova commented Nov 4, 2022

Dynamic Throwing!

  • Added ball interaction with the mouse via a toggle.
  • A user can still choose the original throw ball functionality but if they need a little more interactivity with the ball and their pets, they can toggle on the dynamic option. This allows the user to click and drag on the canvas to throw the ball in any direction.

I really like and enjoy this extension but I felt the need to have more interaction with my pets. Let me know if I breached any formatting for this project or you find any bugs with my code, Ill try to fix them when I can.

This feature contributes to #137

@Luke-G-Cordova
Copy link
Contributor Author

Also, I'm sorry I didn't make an issue for this I totally forgot. If you need me to make one and link it to this PR let me know.

@Luke-G-Cordova Luke-G-Cordova changed the title Dynamic throwing! Added pet interaction, Dynamic throwing! Nov 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2022

Codecov Report

Merging #307 (8bd5049) into master (28959e4) will decrease coverage by 0.95%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   15.06%   14.11%   -0.96%     
==========================================
  Files          17       17              
  Lines         697      744      +47     
  Branches       93      101       +8     
==========================================
  Hits          105      105              
- Misses        592      639      +47     
Impacted Files Coverage Δ
src/panel/main.ts 2.66% <0.00%> (-0.58%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Luke-G-Cordova
Copy link
Contributor Author

@tonybaloney I see that Codecov did not like my pull request, what do I need to fix to get this request merged?

@tonybaloney
Copy link
Owner

Dw about the coverage report.
I've tried to use this and I can't get it working.
I think the 3 commands should be removed and replaced with a single configuration setting. It's confusing to have settings configured via commands.

You can propagate settings via the send message (as a parameter) or when the panel starts up.

@Luke-G-Cordova
Copy link
Contributor Author

@tonybaloney You are totally right about that, thanks for the feedback. I originally constructed the functionality in a way that would depend on the 3 commands but eventually realized it only needed the one and for some reason I just kept the other 2 despite their unneeded complexity. I'll fix it and let you know when it's ready.
As for getting it to work, there should be a button to the right of the throw ball button with the same icon just above the area where you are currently displaying the pets. It is in between the add pet and delete pet buttons currently. This button acts as a toggle and pressing it once allows you to then drag your mouse across the canvas to 'throw' the ball. You can continue to throw the ball in different directions with different velocities by dragging your mouse. This works until you click the toggle again. After this point the extension functions as normal. I will get you a small video demonstration soon to clarify this point more because I can see how it may be confusing to figure out at first because I did not design any special icon or make a style that changes the color of the toggle when it is toggled. Hope this helps for now.

@Luke-G-Cordova Luke-G-Cordova marked this pull request as draft November 7, 2022 10:21
@Luke-G-Cordova
Copy link
Contributor Author

2022-11-07.03-51-23.mov

Every time I am hovering over the icon, I am clicking it. This either enables me to drag my mouse to throw the ball or disables that ability depending on the previous state of that toggle.

@Luke-G-Cordova
Copy link
Contributor Author

I haven't figured out a good way of showing the user that the button is toggled on or off yet because I do not have a lot of experience in the world of vscode extensions yet. I looked at some documentation and decided Id try to find a better solution later. If you have some insight as to how I might go about achieving this goal I'd love to hear it!

@Luke-G-Cordova
Copy link
Contributor Author

Im thinking about taking the button out altogether and just forcing the user to execute the command via the command pallet or binding it to a key binding. What are your thoughts @tonybaloney ?

@Luke-G-Cordova Luke-G-Cordova marked this pull request as ready for review November 8, 2022 00:27
@Luke-G-Cordova
Copy link
Contributor Author

2022-11-07.16-25-56.mov

@tonybaloney I decided to just make it a command instead of also binding the command to a button in the action toolbar. The command is vscode-pets.toggle-dynamic-throw and executing it should enable the ability to throw a ball with the mouse. Executing the command a second time will turn this feature off and allow the extension to operate as normal. I hope this explanation helps and simplifies things! This should be good to merge when you are ready.

@Luke-G-Cordova
Copy link
Contributor Author

@tonybaloney were you ever able to get this working on your local?

@tonybaloney
Copy link
Owner

Sorry, I've been really busy. Got it working this time. Love the behaviour and it makes it a lot more fun to be able to lob the ball sideways.

screenshot 2022-11-20 at 18 10 15

Some feedback--

  1. I don't think it's obvious what "dynamic throwing" means to the user. "Throw ball with mouse" might be more obvious?
  2. The toggled state should be a configuration setting in configuration properties, not a global state within the panel that needs to be toggled by a command. There are a few reasons for this:
  • It's not persistent, if the panel shows/hides it'll reset the state and it's not obvious why you can't suddenly throw with the ball
  • It's not obvious what the current state is (am I toggling it on or off?) unless you throw the ball
  • Every time you restart VS Code (well actually, just close the panel) you have to run the command again

You can reuse the toggle action code by adding a watch to the configuration option, like this. You'll need to have some logic in activate to set the state to whatever is configured by the user.

For (2) I understand that's a bit more work and you've already done a lot to get this feature built, so I can merge as-is and change it to a configuration setting for you if you want?

@Luke-G-Cordova
Copy link
Contributor Author

Luke-G-Cordova commented Nov 20, 2022

Hey I really appreciate your feedback! I think you are right about the "dynamic throwing" name, I chose something quick and easy so I could get to the development stuff. "Throw ball with mouse" or "Throw with mouse" would definitely be more user friendly.

As for the global states, I ran into the same problems you mention. It is my first time working on a vscode extension and I didn't really know how to go about fixing them even after some google searches. I am totally willing to take a stab at it given your new tips but I do feel like you know the organizational flow that this project is going for a little better than I do currently.

I am totally ok if you'd like to merge as-is and change it to a configuration setting! Let me know if you need me to change it to a config setting and Ill do my best. I'm definitely going to look over the code if you do decide to do it because I am curious and it'll help in the future if I contribute more.
Again, thanks for the feedback!

@tonybaloney
Copy link
Owner

@Luke-G-Cordova In that case, I'll merge this and then raise another PR to demonstrate the change and ask for your review on that PR.

@tonybaloney tonybaloney merged commit c5ea3d1 into tonybaloney:master Nov 20, 2022
tonybaloney added a commit that referenced this pull request Feb 26, 2023
Added pet interaction, Dynamic throwing!
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.

3 participants