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

Profile default avatars #579

Merged
merged 5 commits into from Jan 24, 2019

Conversation

Projects
None yet
4 participants
@eoger
Copy link
Contributor

commented Jan 23, 2019

Part of #461

@eoger eoger requested a review from vladikoff Jan 23, 2019

@vladikoff
Copy link
Member

left a comment

lgtm

Show resolved Hide resolved CHANGELOG.md Outdated
Show resolved Hide resolved ...a-client/android/src/main/java/org/mozilla/fxaclient/internal/Profile.kt Outdated
Show resolved Hide resolved components/fxa-client/src/ffi.rs Outdated
Show resolved Hide resolved components/fxa-client/src/ffi.rs Outdated
@thomcc
Copy link
Contributor

left a comment

Looks good, but the bool thing is a problem.

Show resolved Hide resolved CHANGELOG.md Outdated
@rfk

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

How will we track/do the upstream a-c API change, and do we need to resolve any questions around it before merging a breaking API change to master here?

(I'm specifically thinking of the period where we couldn't make a new a-s release for a while because we'd landed some breaking FxA API changes but not yet reflected them into a-c, and what we can do to avoid that in future)

@thomcc

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

Had I known this was incoming I would have waited to include it in 0.14.0 (updating in mozilla-mobile/android-components#1765). But since that should land soon, there's no real problem since nothing else should land thats a breaking change really.

@eoger eoger force-pushed the profile-default-avatars branch from b844465 to f60ae5e Jan 23, 2019

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

@thomcc might reviewing again?
Ran the u8 changes against the iOS and Android sample apps and the boolean was set correctly.

@thomcc

thomcc approved these changes Jan 24, 2019

Copy link
Contributor

left a comment

Looks good, sorry I didn't notice your re-request sooner!

@eoger eoger force-pushed the profile-default-avatars branch from f60ae5e to 60939c9 Jan 24, 2019

@eoger eoger merged commit 1040b6f into master Jan 24, 2019

5 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - beta Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - stable Your tests passed on CircleCI!
Details
ci/circleci: Sync integration tests Your tests passed on CircleCI!
Details

@eoger eoger deleted the profile-default-avatars branch Jan 24, 2019

jrconlin added a commit that referenced this pull request Jan 30, 2019

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