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

Get country from user profile #68

Merged
merged 1 commit into from Oct 18, 2019
Merged

Conversation

svenvNL
Copy link
Contributor

@svenvNL svenvNL commented Oct 15, 2019

Country is used by Spotify to determine which songs can be played by an user. By using the country from the users profile, the user does not have to configure it by themselves and the user will not be surprised when a song is not able to play because they have configured another country then in there Spotify account settings.

@svenvNL svenvNL force-pushed the user-country branch 2 times, most recently from c206d13 to a8874ce Compare October 15, 2019 15:28
Copy link
Owner

@Rigellute Rigellute left a comment

Choose a reason for hiding this comment

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

Thanks for taking a crack at this.

I think we could implement this slightly differently, however.

We could store the whole response from current_user in a user prop within app. This would be useful as we could use more user properties for other features.

To not block startup, this request could be made after first render (check line 223 in main.rs).

Do you want to have a go at this? No worries if not.

And if I'm not clear, please give me a shout.

@svenvNL
Copy link
Contributor Author

svenvNL commented Oct 15, 2019

@Rigellute Thanks for the feedback! I will change the code tomorrow.

@svenvNL svenvNL force-pushed the user-country branch 5 times, most recently from 2947a56 to 0df2dad Compare October 17, 2019 09:42
Copy link
Owner

@Rigellute Rigellute left a comment

Choose a reason for hiding this comment

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

Great work @svenvNL!

Have left a minor style point, up to you if you'd like to address. Will merge a bit later today.

src/app.rs Outdated
Err(e) => {
self.handle_error(e);
}
if let Some(user) = &self.user.to_owned() {
Copy link
Owner

Choose a reason for hiding this comment

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

To avoid one extra layer of nesting, we could create a tuple and match against that.
e.g.

if let (Some(spotify), Some(user)) = (&self.spotify, &self.user.to_owned()) {

}

Ok(result) => {
app.track_table.tracks = result.tracks.items.clone();
app.search_results.tracks = Some(result);
if let Some(user) = app.user.clone() {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here

@svenvNL
Copy link
Contributor Author

svenvNL commented Oct 18, 2019

@Rigellute wasn't aware Rust has this syntax available. I have changed the code.

@Rigellute
Copy link
Owner

Great work @svenvNL!

@Rigellute Rigellute merged commit 2a88f38 into Rigellute:master Oct 18, 2019
@Rigellute
Copy link
Owner

@all-contributors please add @svenvNL for code

@allcontributors
Copy link
Contributor

@Rigellute

I've put up a pull request to add @svenvNL! 🎉

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

2 participants