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

Currency Selection Feature #575

Merged
merged 11 commits into from
Feb 22, 2019
Merged

Currency Selection Feature #575

merged 11 commits into from
Feb 22, 2019

Conversation

justinswart
Copy link
Contributor

@justinswart justinswart commented Feb 4, 2019

📲 What

Replaces the currency picker behaviour with selection from a table view.

Before After
2019-02-21 14 16 25 2019-02-20 16 02 10

🤔 Why

The previous behaviour was a little tricky to use and switching to a list felt like a better user experience and more consistent with the rest of our settings implementations.

🛠 How

  • Removed the currency picker, replaced with a UITableView.
  • Changed the transition to push the table view onto the navigation stack.

👀 See

Trello

♿️ Accessibility

  • Tap targets use minimum of 44x44 pts dimensions
  • Works with VoiceOver
  • Supports Dynamic Type

✅ Acceptance criteria

TBD

⏰ TODO

@justinswart
Copy link
Contributor Author

@justinswart justinswart force-pushed the feature-currency-selection branch from 88b4173 to bf1ea70 Compare February 7, 2019 21:18
* Remove currency picker

* swiftlint

* Remove redundant enum case

* Remove reference
@justinswart justinswart force-pushed the feature-currency-selection branch from bf1ea70 to 59fb231 Compare February 7, 2019 23:11
* Remove currency picker

* swiftlint

* Add SelectCurrencyViewController

* Fix tests

* Update snapshots

* Remove redundant enum case

* Remove reference

* Remove SettingsCurrencyCell

* Fix snapshot tests

* Remove CaseIterable conformance on SettingsAccountCellType

* Use reuseIdentifier local constant

* Update pragma

* Update tableview instantiation

* Set nav title

* Fix yet another merge issue

* Fix merge
@justinswart justinswart added the blocked a PR that is blocked for external reasons label Feb 14, 2019
# Conflicts:
#	Kickstarter-iOS/Library/Nib.swift
#	Library/ViewModels/SettingsAccountViewModel.swift
* Add SelectCurrencyViewController

* Fix tests

* Add tableview header

* Update snapshots

* Added ViewController tests

* Removed recordMode line from test

* Simplify layout code for header

* Remove SettingsCurrencyCell

* Fix snapshot tests

* Remove CaseIterable conformance on SettingsAccountCellType

* Use reuseIdentifier local constant

* Update pragma

* Update tableview instantiation

* Implicit returns in lazy vars

* Fix alignment

* ① [Currency selection] Remove currency picker (#568)

* Remove currency picker

* swiftlint

* Remove redundant enum case

* Remove reference

* Remove local var, improve function name

* Set label colour

* Update snapshots

* Fix project file

* Correctly size header/footer views

* Make the tests pass (#585)

* Run snapshot tests on all devices

* Remove duplicate extension, use keypath setter

* Fix autolayout warnings
* Remove currency picker

* swiftlint

* Add SelectCurrencyViewController

* Fix tests

* Update snapshots

* Add tableview header

* Added ViewController tests

* Removed recordMode line from test

* Simplify layout code for header

* Add update calls

* swiftlint

* Add update calls

* Add view model tests

* Remove stray file ref

* Fix tests

* swiftlint

* Remove redundant enum case

* Fix rebase duplication

* Remove reference

* Remove SettingsCurrencyCell

* Fix snapshot tests

* Remove CaseIterable conformance on SettingsAccountCellType

* Use reuseIdentifier local constant

* Update pragma

* Update tableview instantiation

* Implicit returns in lazy vars

* Fix alignment

* Add SelectCurrencyViewController

* Fix tests

* Add tableview header

* Update snapshots

* Added ViewController tests

* Removed recordMode line from test

* Simplify layout code for header

* Remove SettingsCurrencyCell

* Fix snapshot tests

* Remove CaseIterable conformance on SettingsAccountCellType

* Use reuseIdentifier local constant

* Update pragma

* Update tableview instantiation

* Implicit returns in lazy vars

* Fix alignment

* ① [Currency selection] Remove currency picker (#568)

* Remove currency picker

* swiftlint

* Remove redundant enum case

* Remove reference

* Remove local var, improve function name

* Set label colour

* Update snapshots

* Fix project file

* Correctly size header/footer views

* Make the tests pass (#585)

* Fix merge issue

* Run snapshot tests on all devices

* Remove duplicate extension, use keypath setter

* Fix autolayout warnings

* Fix merge issue

* Add weak self, fix project file

* Renamed signal

* Refactor view model and tests

* Use more inference
@justinswart justinswart changed the title [WIP] Currency Selection Feature Currency Selection Feature Feb 21, 2019
@justinswart justinswart added needs review and removed blocked a PR that is blocked for external reasons labels Feb 21, 2019
@justinswart justinswart assigned dusi and ifbarrera and unassigned dusi Feb 21, 2019
@justinswart
Copy link
Contributor Author

@ifbarrera added success banner:

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

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

:shipit:

@justinswart justinswart merged commit 337b31f into master Feb 22, 2019
@justinswart justinswart deleted the feature-currency-selection branch February 22, 2019 19:30
@justinswart justinswart mentioned this pull request Feb 26, 2019
2 tasks
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.

3 participants