-
Notifications
You must be signed in to change notification settings - Fork 5.6k
[ACTION] Clubhouse.io (now Shortcut) #1383
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
[ACTION] Clubhouse.io (now Shortcut) #1383
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). pipedream-docs-redirect-do-not-edit – ./docs🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs-redirect-do-not-edit/9YrZKUkvSS3shH2tFVznm6grohKg pipedream-docs – ./docs🔍 Inspect: https://vercel.com/pipedreamers/pipedream-docs/CN7PVurxgJzoQBvy6rTY1sMV7L4M |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergioeliot2039 thank you for this! Per the Guidelines, please make the following changes:
- Rebase off master
- Fix all eslint issues
- Refactor all props that are used more than once into the app file
- Refactor all API access code to use the clubhouse-lib where possible, unless there is a good reason not to do so (please explain in detail wherever this is the case)
- Add
options()methods wherever possible to all ID props to populate options (epicId,groupId, etc)
|
@compwright when i started developing components, the criteria to use a library was:
in the case for if you check stats in npm for clubhouse-lib the lib is official and has ~3k weekly download, but last update was over 5 months ago. that's why I didn't use it. another parameter I use is that usually PD team will use the library in Legacy actions' "Run node.js code with CLubhouse", and they are using the API directly. |
|
@sergioeliot2039 I think we can be a little loose on #2 and #3 if it's an official library. It looks like the maintainers are pushing new releases every so often. It's probably the case that the API hasn't changed in the last few months. This is a tough call but in this specific case I'd use the npm package. Clubhouse isn't the largest app, and that's part of the reason why I think the number of downloads is low. |
|
@jcortes this ready for a new review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of suggestions and you are good to go! Thanks for making those changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I forgot my last comment about returning the statusCode not as an array but as a single value.
jcortes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me!
|
The Create Story action functionally works, but it returns the below |
|
@sergioeliot2039 I pushed a few changes and created a new PR from the pipedream repo, so it's in sync w/ master |
|
@sergioeliot2039 is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
|
Looks good, @sergioeliot2039 ! I see there are some Lint errors you'll need to address before merging. |
|
Great, @dylburger . |
PR for Clubhouse actions
Create Story
Search Stories