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

Readme fix, added JSDoc string, Fixed updateActionData min/max Value when 0 raise error, and Don't validate removeState #26

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

KillerBOSS2019
Copy link

  • Fixed readme ( fully working JS example )
  • Added doc string for almost all of methods that describe args and return.
  • Added removeStateMany()
  • Fixed connect() so it uses arg instead of object
  • Fixed CheckForUpdate() instead of using parent variable it's an arg.

- Fixed readme ( fully working JS example )
- Added doc string for almost all of methods that describe args and return.
- Added removeStateMany()
- Fixed connect() so it uses arg instead of object
- Fixed CheckForUpdate() instead of using parent variable it's an arg.
spdermn02
spdermn02 previously approved these changes Feb 14, 2023
src/client.js Outdated Show resolved Hide resolved
Copy link
Owner

@spdermn02 spdermn02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missed the minor comment i made. fix that and then we can test it out and get it merged.

@spdermn02
Copy link
Owner

Also sorry this took so long for me to review - I need to start using the Volta page more often. need to use that to keep track of all the stuff i need to work on for my TP plugins.

@KillerBOSS2019
Copy link
Author

Ah sorry I completely forgot about this. I'll fix that when I get on my PC.

src/client.js Outdated Show resolved Hide resolved
@mpaperno
Copy link
Collaborator

Good revert. But your merge clobbered #33 :-(

@KillerBOSS2019
Copy link
Author

I hate conflict lol

@mpaperno
Copy link
Collaborator

mpaperno commented Aug 12, 2023

Conflict sucks!

☮️

@KillerBOSS2019
Copy link
Author

Okay.... hope that solves it.

src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
src/client.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@mpaperno mpaperno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@KillerBOSS2019
Copy link
Author

YES FINALLY PASSED one more to go hehe

src/client.js Outdated Show resolved Hide resolved
@KillerBOSS2019 KillerBOSS2019 changed the title Readme fix, added JSDoc string Readme fix, added JSDoc string, Fixed updateActionData min/max Value when 0 raise error, and Don't validate removeState Aug 12, 2023
mpaperno
mpaperno previously approved these changes Aug 12, 2023
Copy link
Owner

@spdermn02 spdermn02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my note

src/client.js Outdated Show resolved Hide resolved
spdermn02
spdermn02 previously approved these changes Sep 1, 2023
@mpaperno
Copy link
Collaborator

mpaperno commented Nov 9, 2023

Hi Damien,

I fixed the conflicts, but TBH it would be great to just add the JS Docs as a standalone commit, w/out any functional changes. This adds real value on its own, especially with the strong VSCode/IDE type checking these days. I'll probably do something with those myself soon if no one else does...

The README example fix would make sense in the same PR, but as its own commit (maybe it already is but I lost track... :) ).

The update check changes are a fine fix/simplification but should be their on their own clean commit, at least, if not PR. Maybe even a good place to "sneak in" removing the useless this.touchPortal as well... ;)

And the state update helper... I'm ambivalent about that, but at the very least this part actually needs to be tested (and as far as I can tell no one has). Mixing it with unrelated changes is the main issue I have with this PR at this point.

Thanks!
-Max

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

3 participants