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

Adding Latitude and Longitude to Twitter Status Update... #714

Merged
merged 6 commits into from Dec 15, 2016

Conversation

Projects
None yet
4 participants
@SpilledMilkCOM
Contributor

SpilledMilkCOM commented Dec 13, 2016

This is my first "official" PR so I HOPE I'm following the rules. I REALLY would like the Lat/Long added to the status. This looks like it's working through your test application. I did not see any Unit Tests for Twitter otherwise I would have added some. In the account that I tested with I only see "Colorado, USA" versus the lat/long (which I think used to be displayed last year or so). That might be a setting on a per account basis. As I was doing my research and testing, it didn't take much to add the REST of the UPDATE parameters through the mechanism I chose. It also lends itself for extension as well without changing the provider. I created a NEW method so I would not break the API, and then under the covers call the new API from the old API. I hope this is all I need to do so I can continue with my UWP application that used to use TweetSharp in the old Windows Phone 8.1 world. THANKS!! I hope this helps! Thank YOU for all of the other plumbing and reducing my work. So I thought I would give back a few hours...

Parker Smart added some commits Dec 13, 2016

Parker Smart Parker Smart
Add Latitude and Longitude to a Tweet. Implementing another method so…
… as to NOT break the existing API. Also updated the sample app for testing purposes.
@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Dec 13, 2016

Hi @SpilledMilkCOM, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas commented Dec 13, 2016

Hi @SpilledMilkCOM, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas

This comment has been minimized.

Show comment
Hide comment
@dnfclas

dnfclas Dec 13, 2016

@SpilledMilkCOM, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

dnfclas commented Dec 13, 2016

@SpilledMilkCOM, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@dnfclas dnfclas added cla-signed and removed cla-required labels Dec 13, 2016

@deltakosh

This is what I can call an almost perfect PR :) I would just ask for a documentation update to mention the great features you add! (And perhaps add a new line in the .bind to show users they can tweet a text OR a status)

@deltakosh

This comment has been minimized.

Show comment
Hide comment
@deltakosh

deltakosh Dec 13, 2016

Contributor

A cool option could be to automatically (under an option) get the device coordinates :)

Contributor

deltakosh commented Dec 13, 2016

A cool option could be to automatically (under an option) get the device coordinates :)

@SpilledMilkCOM

This comment has been minimized.

Show comment
Hide comment
@SpilledMilkCOM

SpilledMilkCOM Dec 14, 2016

Contributor

I'll have to dig around and see where I can update the documentation... Unless you want to let me know where that is here. I wasn't sure how to do that. I seriously WAS going to add the Geolocation, BUT you have to adjust the project manifest file to include that capability which I didn't figure anybody would WANT me to do. I will DEFINITELY DO that though. I already have the code to get the lat/long so I will add it to the sample application. I don't think that the Twitter namespace should rely on the Geolocation stuff since it should be straight Twitter, but adding it to the sample application would show people how they can do that easily. Since you want to display this lat/long anyway in your UI, you would be getting that value externally versus grabbing it from a Twitter toolkit class. I'll whip something up so you can see it.

Contributor

SpilledMilkCOM commented Dec 14, 2016

I'll have to dig around and see where I can update the documentation... Unless you want to let me know where that is here. I wasn't sure how to do that. I seriously WAS going to add the Geolocation, BUT you have to adjust the project manifest file to include that capability which I didn't figure anybody would WANT me to do. I will DEFINITELY DO that though. I already have the code to get the lat/long so I will add it to the sample application. I don't think that the Twitter namespace should rely on the Geolocation stuff since it should be straight Twitter, but adding it to the sample application would show people how they can do that easily. Since you want to display this lat/long anyway in your UI, you would be getting that value externally versus grabbing it from a Twitter toolkit class. I'll whip something up so you can see it.

@SpilledMilkCOM

This comment has been minimized.

Show comment
Hide comment
@SpilledMilkCOM

SpilledMilkCOM Dec 14, 2016

Contributor

Looks like the PR updated automagically. Let me know if there are problems. Hopefully this makes it into the official release SOON! I'd LOVE to get this out with my latest UWP converted app.

I apologize for not "discussing" this in the forums first, but I needed the lat/long stuff and I figured other people would use it too. (Plus I didn't read the official documentation until I found the folder and started to browse it.) THEN I just figured I'd add the rest of the params since the TwitterStatus class is so easy. I'm glad somebody ELSE did the media. :) That was a pain using TweetSharp, but doable.

Contributor

SpilledMilkCOM commented Dec 14, 2016

Looks like the PR updated automagically. Let me know if there are problems. Hopefully this makes it into the official release SOON! I'd LOVE to get this out with my latest UWP converted app.

I apologize for not "discussing" this in the forums first, but I needed the lat/long stuff and I figured other people would use it too. (Plus I didn't read the official documentation until I found the folder and started to browse it.) THEN I just figured I'd add the rest of the params since the TwitterStatus class is so easy. I'm glad somebody ELSE did the media. :) That was a pain using TweetSharp, but doable.

@deltakosh

This comment has been minimized.

Show comment
Hide comment
@deltakosh

deltakosh Dec 14, 2016

Contributor

No need for apologize :) this is really good stuff
I'ok check and merge asap:)

Contributor

deltakosh commented Dec 14, 2016

No need for apologize :) this is really good stuff
I'ok check and merge asap:)

@deltakosh

This comment has been minimized.

Show comment
Hide comment
@deltakosh

deltakosh Dec 14, 2016

Contributor

Btw I did the media and it was painful:)
As soon as we will merge you will be able to use the prerelease repo to get a nuget package for your app. Hopefully tomorrow

Contributor

deltakosh commented Dec 14, 2016

Btw I did the media and it was painful:)
As soon as we will merge you will be able to use the prerelease repo to get a nuget package for your app. Hopefully tomorrow

@SpilledMilkCOM

This comment has been minimized.

Show comment
Hide comment
@SpilledMilkCOM

SpilledMilkCOM Dec 14, 2016

Contributor

THANK YOU!! I'll play with the sample app over my vacation and see what I can help with...

Contributor

SpilledMilkCOM commented Dec 14, 2016

THANK YOU!! I'll play with the sample app over my vacation and see what I can help with...

@deltakosh

This comment has been minimized.

Show comment
Hide comment
@deltakosh

deltakosh Dec 14, 2016

Contributor

Perfect.Just waiting for a second reviewer and we can merge!

Contributor

deltakosh commented Dec 14, 2016

Perfect.Just waiting for a second reviewer and we can merge!

Show outdated Hide outdated ...ft.Toolkit.Uwp.SampleApp/SamplePages/Twitter Service/TwitterPage.xaml.cs
Show outdated Hide outdated Microsoft.Toolkit.Uwp.Services/Services/Twitter/TwitterStatus.cs
Show outdated Hide outdated Microsoft.Toolkit.Uwp.Services/Services/Twitter/TwitterStatus.cs
Show outdated Hide outdated Microsoft.Toolkit.Uwp.Services/Services/Twitter/TwitterStatus.cs
Show outdated Hide outdated Microsoft.Toolkit.Uwp.Services/Services/Twitter/TwitterStatus.cs
Show outdated Hide outdated Microsoft.Toolkit.Uwp.Services/Services/Twitter/TwitterStatus.cs
if (value)
{
result = $"{result}&{parameterName}=true";

This comment has been minimized.

@dotMorten

dotMorten Dec 14, 2016

Collaborator

Why only true? What if the default is true and you want to force it to false?

@dotMorten

dotMorten Dec 14, 2016

Collaborator

Why only true? What if the default is true and you want to force it to false?

This comment has been minimized.

@SpilledMilkCOM

SpilledMilkCOM Dec 15, 2016

Contributor

I don't believe any of their settings are defaulted to be true. Twitter Update Reference They've named their Boolean parameters in such a way that setting them to true turns the feature on. By omitting them they will stay false. I suppose I could send false, but I think that would be redundant.

@SpilledMilkCOM

SpilledMilkCOM Dec 15, 2016

Contributor

I don't believe any of their settings are defaulted to be true. Twitter Update Reference They've named their Boolean parameters in such a way that setting them to true turns the feature on. By omitting them they will stay false. I suppose I could send false, but I think that would be redundant.

This comment has been minimized.

@dotMorten

dotMorten Dec 15, 2016

Collaborator

OK

@dotMorten

dotMorten Dec 15, 2016

Collaborator

OK

Show outdated Hide outdated Microsoft.Toolkit.Uwp.Services/Services/Twitter/TwitterStatus.cs
@dotMorten

Looks good to me now

@deltakosh deltakosh merged commit bb22a72 into Microsoft:dev Dec 15, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment