Skip to content

Account Settings, part 1 - #4740

Merged
koke merged 4 commits into
developfrom
feature/account-settings-take-2
Feb 8, 2016
Merged

Account Settings, part 1#4740
koke merged 4 commits into
developfrom
feature/account-settings-take-2

Conversation

@koke

@koke koke commented Jan 27, 2016

Copy link
Copy Markdown
Member

To review after #4732, relative diff

I just ported the code that I already had in the feature/account-settings branch to the latest design from #4732. I started writing the account settings feature, then I decided to first implement all the RxSwift stuff on My Profile first, then extract the common logic.

This isn't the full feature, but the easy parts 😉

  • Web address doesn't have any validation, it's just a plain text field.
  • Email is read only, since it needs validation, and a whole flow for email change confirmation.
  • Primary site selector is missing
  • The mockups also had Interface Language and Instant Post Feedback, which I argued don't make much sense on the app, since they feel more like client settings for calypso.

Needs Review: @astralbodies

@koke koke added the /Me label Jan 27, 2016
@koke koke added this to the 6.0 milestone Jan 27, 2016
@koke

koke commented Jan 27, 2016

Copy link
Copy Markdown
Member Author

Account Settings

@astralbodies astralbodies self-assigned this Jan 31, 2016
@astralbodies

Copy link
Copy Markdown
Contributor

Now that I got the My Profile network PR reviewed, I'll review this one shortly.

@astralbodies

Copy link
Copy Markdown
Contributor

Reviewing now, @koke. Sorry for the delay. Can you bring the branch current with develop in the meantime?

@astralbodies

Copy link
Copy Markdown
Contributor

SettingsViewControllerTest.m needs to get removed - the file wasn't being compiled because it wasn't part of the test target 😕

@koke

koke commented Feb 5, 2016

Copy link
Copy Markdown
Member Author

Merged and removed test

@astralbodies

Copy link
Copy Markdown
Contributor

I'm still wary of how the failed updates are handled with the data flopping back to the original value. In the end its not destructive and at least the UI does reflect the reverted data. Maybe we can put an issue (if there isn't one already) for coming up with a way to signal a failed update if the user is still in the screen where the notification is relevant.

One side note I didn't see the activity indicator in the status bar when changing the web address and tapping done with a bad network link conditioner turned on. I went back to Me and then saw the spinner.

Otherwise @koke this looks solid 👊

:shipit:

koke added a commit that referenced this pull request Feb 8, 2016
@koke
koke merged commit cc16908 into develop Feb 8, 2016
@koke
koke deleted the feature/account-settings-take-2 branch February 8, 2016 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants