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

[NT-328] Add min pledge label to pledge screen #871

Merged
merged 7 commits into from
Oct 4, 2019
Merged

Conversation

dusi
Copy link
Contributor

@dusi dusi commented Oct 3, 2019

📲 What

Adds min pledge amount label with description of what the minimum amount is.

🤔 Why

This is a product requirement.

🛠 How

Simply adds label and shows/hides it based on whether it is a reward or no reward (hidden for no rewards).

👀 See

Before 🐛 After (no reward) 🦋 After (reward) 🦋
Screen Shot 2019-10-03 at 11 52 50 AM Screen Shot 2019-10-03 at 11 52 50 AM Screen Shot 2019-10-03 at 11 52 54 AM

♿️ Accessibility

  • Works with VoiceOver
  • Supports Dynamic Type

✅ Acceptance criteria

  • Min pledge label should SHOW and show the correct amount & currency for reward (on the
    pledge screen)
  • Min pledge label should NOT SHOW for no reward (on the pledge screen)

⏰ TODO

  • Use generated string when it's available

@dusi dusi changed the title Add min pledge label [NT-328] Add min pledge label to pledge screen Oct 3, 2019
Copy link
Contributor

@Scollaco Scollaco left a comment

Choose a reason for hiding this comment

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

Nice @dusi ! I just left a couple of minor comments 🥇

minValue
)
.map { project, min in
localizedString(
Copy link
Contributor

Choose a reason for hiding this comment

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

The string (untranslated) seems to be available. So I think we can replace this line with

Strings.The_minimum_pledge_is(min_pledge: Format.currency(min, country: project.country, omitCurrencyCode: false))

self.titleLabel,
self.adaptableStackView,
self.minPledgeAmountLabel,
UIView(frame: .zero)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that this was already merged, but I wonder if creating a variable to better describe what exactly the UIView(frame: .zero) is would be more readable.
Something like:

let spacer (or whatever name) = UIView(frame: .zero)

_ = ([
      self.titleLabel,
      self.adaptableStackView,
      self.minPledgeAmountLabel,
      spacer
    ], self.rootStackView)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a prop

@dusi dusi requested a review from Scollaco October 4, 2019 15:33
Copy link
Contributor

@Scollaco Scollaco left a comment

Choose a reason for hiding this comment

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

GIF

@dusi dusi merged commit d73f37b into master Oct 4, 2019
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.

2 participants