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

Add support for Meetup.com #23

Merged
merged 1 commit into from Dec 28, 2019
Merged

Add support for Meetup.com #23

merged 1 commit into from Dec 28, 2019

Conversation

@jamietanna
Copy link
Contributor

jamietanna commented Dec 26, 2019

As part of snarfed/bridgy/issues/873 we want to add Meetup.com support
to Brid.gy.

Before we can do this, we need to onboard it to oauth-dropins.

We can do this by largely duplicating the code from github.py and
dropbox.py, amending a couple of things for the Meetup-specific flow.

Note: This is a draft because I've been unable to get it deployed successfully to App Engine and verify it.

@jamietanna

This comment has been minimized.

Copy link
Contributor Author

jamietanna commented Dec 27, 2019

I've been unable to get the dev_appserver.py setup working as it requires some tweaks for local grpcio setup

But without that I've had some issues getting the image to work - any thoughts?

Copy link
Owner

snarfed left a comment

awesome! excited for this. i left a few comments. let me know if you need any more help getting dev_appserver working!

oauth_dropins/meetup.py Outdated Show resolved Hide resolved
oauth_dropins/meetup.py Outdated Show resolved Hide resolved
oauth_dropins/meetup.py Show resolved Hide resolved
oauth_dropins/meetup.py Show resolved Hide resolved
oauth_dropins/meetup.py Outdated Show resolved Hide resolved
oauth_dropins/meetup.py Outdated Show resolved Hide resolved
@jamietanna

This comment has been minimized.

Copy link
Contributor Author

jamietanna commented Dec 27, 2019

Okey dokey, this is now working - just got a few comments from you to address and a couple of TODOs from me. I'll push the final changes later - if we're happy then I'll clean up commit history and mark this PR as Ready for Review!

@jamietanna jamietanna force-pushed the jamietanna:feature/meetup branch 2 times, most recently from be3921c to bda4b13 Dec 27, 2019
@jamietanna

This comment has been minimized.

Copy link
Contributor Author

jamietanna commented Dec 27, 2019

@snarfed looks like we should now be ready for final reviews! I've unfortunately not yet got the OAuth Consumer approved for oauth-dropins on Meetup, but I can share the Brid.dy one temporarily?

@snarfed

This comment has been minimized.

Copy link
Owner

snarfed commented Dec 27, 2019

meetup_2x.png is great!...but too big to actually serve as a button. 😁 mind renaming it to something like meetup_full.png, and resize to 80px high in meetup_2x.png? (since the buttons are all 100px tall, and you're including 10px padding.)

otherwise this is looking great!

@snarfed

This comment has been minimized.

Copy link
Owner

snarfed commented Dec 27, 2019

er, sorry, i guess 60px high, not 80px? or 5px padding instead of 10. the buttons render at 50px tall, and the image should be 2x resolution, so whatever looks good based on that.

@jamietanna

This comment has been minimized.

Copy link
Contributor Author

jamietanna commented Dec 27, 2019

How does this look with it resized?

2019-12-27-190429_1288x755_scrot

@snarfed

This comment has been minimized.

Copy link
Owner

snarfed commented Dec 27, 2019

looks good, thanks!

@jamietanna jamietanna force-pushed the jamietanna:feature/meetup branch from 3bff58e to 4c39e1b Dec 27, 2019
@jamietanna jamietanna marked this pull request as ready for review Dec 27, 2019
@jamietanna

This comment has been minimized.

Copy link
Contributor Author

jamietanna commented Dec 27, 2019

Awesome, that's now ready for review 👍

Copy link
Owner

snarfed left a comment

that's the last thing i see!

oauth_dropins/meetup.py Outdated Show resolved Hide resolved
As part of snarfed/bridgy/issues/873 we want to add Meetup.com support
to Brid.gy.

Before we can do this, we need to onboard it to oauth-dropins.

We can do this by largely duplicating the code from github.py and
dropbox.py, amending a couple of things for the Meetup-specific flow.
@jamietanna jamietanna force-pushed the jamietanna:feature/meetup branch from 4c39e1b to 1be8ca0 Dec 28, 2019
@jamietanna jamietanna requested a review from snarfed Dec 28, 2019
@snarfed

This comment has been minimized.

Copy link
Owner

snarfed commented Dec 28, 2019

thanks!

you mentioned that you have an approved API client for Bridgy. have you tried this with that? does it work? if so, looks ready to merge to me.

@jamietanna

This comment has been minimized.

Copy link
Contributor Author

jamietanna commented Dec 28, 2019

@snarfed yep, I've tested it with that client and it works 👍🏽

How shall I send the client credentials to you? I guess we'll need it before merged / deployed to app engine.

And are we OK using a slightly different client for now?

@snarfed

This comment has been minimized.

Copy link
Owner

snarfed commented Dec 28, 2019

great! it didn't look like you could add other Meetup users to an existing client to administer it, right? if that's true, I'll need to make my own API client, so don't worry about sending yours. feel free to move on to granary!

@snarfed snarfed merged commit e668fad into snarfed:master Dec 28, 2019
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@jamietanna

This comment has been minimized.

Copy link
Contributor Author

jamietanna commented Dec 28, 2019

Fair enough - it takes some time to get a new client registered as its a manual review process, so may be worth using the one I've got for now.

Also may be worth creating one for granary / bridgy now so they're up by the time my PRs are in 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.