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

[Enhancement] - User can successfully change Email #341

Merged
merged 24 commits into from
Sep 25, 2018
Merged

Conversation

Rcureton
Copy link
Contributor

What

  • User can successfully change email. The UI is accurate if you switch to production the Gif is on staging.
  • Created an extension function for setting the custom Snackbar background.
  • Merged in changes from master and a prior branch hence all the file changes.

Story

See 👀

change_email

Rashad Cureton and others added 15 commits September 11, 2018 12:16
* completed screen for change email

* added proper dimens from dimens.xml

* fixed pr feedback
* added change password screen, toolbar and added style for textinputlayout

* refactored styles
* added cells for edit profile and change password

* made rows for account in settings and added account activity layout with rows

* added toolbar layout, account layout, AccountActivity, Updated SettingsNewViewModel

* added edit text and line restrictions

* re-added tests that were deleted accidentally

* fixed lines and spacing issues

* fixed imports

* fixed more imports

* moved the tests into the right package

* setup edit profile cell and added the activity

* added toolbar and imageview to edit profile

* still fixing layout

* updated strings, completed edit profile ui

* removed duplicate activity in manifest and sample layout

* fixed circle issues

* added icon for different screen mods

* fixed error

* fixed more errors

* fixed the last error
@Rcureton Rcureton changed the base branch from settings-v2 to master September 25, 2018 18:20
Copy link
Contributor

@eoji eoji left a comment

Choose a reason for hiding this comment

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

I have questions 👀

this.view.layoutParams = params
this.view.background = ContextCompat.getDrawable(context, R.drawable.change_email_success_bg)
ViewCompat.setElevation(this.view, 6f)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nl

import com.kickstarter.R

fun Snackbar.success(context: Context) {
val params = this.view.layoutParams as ViewGroup.MarginLayoutParams
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic numbers make code harder to understand.
Let's try something like this instead:

    val grid1 = context.resources.getDimensionPixelSize(R.dimen.grid_1)
    val grid2 = context.resources.getDimensionPixelSize(R.dimen.grid_2)
    params.setMargins(grid1, 0, grid1, grid2)

@@ -12,6 +17,9 @@ import rx.subjects.PublishSubject
interface ChangeEmailViewModel {

interface Inputs {
/** Call when the make network call button has been clicked. */
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like copy pasta 👀 🍝

/** Emits a boolean that determines if a network call is in progress. */
fun showProgressBar(): Observable<Boolean>

/** Emits the logged in user's email which we use get the success of the mutation. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you doing anything with this value?

android:shape="rectangle">
<solid android:color="@color/ksr_cobalt_500" />
<corners android:radius="@dimen/grid_1" />
</shape>
Copy link
Contributor

Choose a reason for hiding this comment

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

nl


class ChangeEmailViewModelTest : KSRobolectricTestCase() {

private lateinit var vm: TestApolloViewModel.ViewModel
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be ChangeEmailViewModel.ViewModel?

Copy link
Contributor

@eoji eoji left a comment

Choose a reason for hiding this comment

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

I think it's G2G 🚀

@Rcureton Rcureton merged commit 15e207c into master Sep 25, 2018
@Rcureton Rcureton deleted the rc/email-change branch September 25, 2018 20:42
@Rcureton
Copy link
Contributor Author

Email Update: Success

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants