Proj of the day label #205

bormind · 2017-07-13T20:25:32Z

What

Removing heart icon form the Project of the day label.

Why

Quotation trello card:
"When we add hearts to project cards, we'll have two hearts on a single discovery card.
After discussing with Maggie, we'll just remove the heart from the PotD label for iOS."

How

Set iconImage property while initializing PostcardMetadataData to nil for the PostcardMetadataDataType.potd
After the change realized that we don't have tests for DiscoveryPostCard for featured and potD projects. Implemented DiscoveryPageViewControllerTests.testView_Card_Project_TodaySpecial test. Generated reference images for featured and potD post cards

See 👀

Trello

theginbin

Looking good Boris! Thanks for adding tests for the metadata. I noticed that padding is greater on the left of the "Project of the Day" text where the imageView would be. Can you try hiding the imageView in DiscoveryPostcardCell if the image is nil to see if that fixes that extra space?

theginbin · 2017-07-13T22:40:41Z

Kickstarter-iOS/Views/Controllers/DiscoveryPageViewControllerTests.swift

  func testView_Card_Project() {
    let projectTemplate = anomalisaNoPhoto
-      |> Project.lens.dates.deadline .~ (self.dateType.init().timeIntervalSince1970 + 60 * 60 * 24 * 6)


is this just deleted because it is unused?

Yes, this is not used in the test and doesn't change the output screen

theginbin · 2017-07-13T22:44:16Z

Kickstarter-iOS/Views/Controllers/DiscoveryPageViewControllerTests.swift

+
+          FBSnapshotVerifyView(parent.view,
+                               identifier: "\(labeledProj.0)_lang_\(language)_device_\(device)")
+        }


thanks for adding these tests!

bormind · 2017-07-13T22:53:31Z

Removed non used "metadata-podt" icon from assets

bormind · 2017-07-13T23:22:19Z

Gina, great catch with shifted labels!
I fixed it by hiding image ctrl if icon is not present as you suggested.

justinswart

Just one comment and then also just noticed that one one or two of the french language screenshots padding seems bigger on the right: https://github.com/kickstarter/ios-oss/pull/205/files#diff-55f39769cd8f7cd3f48185dadd94e9b1

Is there perhaps a min-width on that container?

justinswart · 2017-07-14T08:25:14Z

Kickstarter-iOS/Views/Cells/DiscoveryPostcardCell.swift

@@ -189,6 +189,7 @@ internal final class DiscoveryPostcardCell: UITableViewCell, ValueCell {
    self.viewModel.outputs.metadataData
      .observeForUI()
      .observeValues { [weak self] data in
+        self?.metadataIconImageView.isHidden = data.iconImage == nil


@theginbin any idea why so many this was originally done with so many side-effects in a single observeValues?. I think if we did this today we'd have had an output for each of those and been more explicit about the logic in the view model.

@justinswart, @theginbin, I noticed it too. I don't see problem with this approach but it is different from our "regular way" of doing things. I would be very interested to have a short discussion on why having only one side-effect per signal is a better approach..

@justinswart you're totally right. I have no idea why this wasn't broken out into several outputs. It may have been built very early or done out of time constraint at the time before shipping. @bormind having only one side effect per signal makes it easier to test and understand exactly what is being changed (for example, label text versus just knowing that an entire piece of data changed)

@theginbin this is why I wanted to have conversation about this. I actually disagree that It will be easier to test. I understand that change on any one of the data properties causes signal of entire data change. But because the data unit consist of contextually dependent properties, I believe having one signal for dat unit change will be simpler to describe in the unit test and easier to understand where actual problem is in case the bug happened.

I guess by "easer to test and understand", I mean that a unique output tells us exactly what we're expecting to update on the UI specifically. That's why we typically don't have one output to update the entire view because that doesn't tell us what we're doing with individual UI components.

theginbin · 2017-07-14T16:44:20Z

approving this! but if you feel up for it, it would be good to add a new output for that hidden imageView and a test with it. It's good practice anyway.

bormind · 2017-07-14T17:39:09Z

Thank you @theginbin! I believe that if we decide to create output signal for imageView.isHidden property then should create output signals for all other meta properties:

self?.metadataIconImageView.image = data.iconImage
self?.metadataLabel.text = data.labelText
self?.metadataIconImageView.tintColor = data.iconAndTextColor
self?.metadataLabel.textColor = data.iconAndTextColor

theginbin · 2017-07-14T19:10:16Z

@bormind so yeah I think you should go ahead and create outputs for all of them. By having separate outputs, we give the view model context about what is happening with the data. With a single output, we lose this context.

justinswart · 2017-07-17T11:48:56Z

@bormind if you're feeling up to it it might be a useful exercise to break it up into separate outputs and write the corresponding tests! I realize it adds a fair bit of code to what was otherwise a pretty small change but it would be good for consistency with the rest of our code 👍

justinswart · 2017-07-24T19:14:49Z

@bormind hey since your last commit separating out the metadataIconHidden into its own signal, did you not want to remove self?.metadataIconImageView.isHidden = data.iconImage == nil from the observeValues on self.viewModel.outputs.metadataData?

Also, I think the idea was that we would refactor this to be separate outputs for each metadata property.

justinswart · 2017-07-26T19:53:40Z

@bormind hey just checking if you saw my last comment? Also I think we were still going to split this out into multiple outputs?

You said before on the PR:

Thank you @theginbin! I believe that if we decide to create output signal for imageView.isHidden property then should create output signals for all other meta properties

justinswart

Thanks for splitting out into separate outputs! Just one or two spelling comments and a question!

justinswart · 2017-07-31T14:47:05Z

Library/ViewModels/DiscoveryPostcardViewModel.swift

@@ -76,8 +76,20 @@ public protocol DiscoveryPostcardViewModelOutputs {
  /// Emits a boolean to determine whether or not to display funding progress container view.
  var fundingProgressContainerViewHidden: Signal<Bool, NoError> { get }

-  /// Emits the disparate data to be displayed on the metadata view label.
-  var metadataData: Signal<PostcardMetadataData, NoError> { get }
+  /// Emits matadata label text


justinswart · 2017-07-31T14:48:28Z

Library/ViewModels/DiscoveryPostcardViewModel.swift

+  /// Emits matadata label text
+  var metadataLabelText: Signal<String, NoError> { get }
+
+  /// Emits metadata icon name


I think this should say:

/// Emits metadata icon image

justinswart · 2017-07-31T14:50:41Z

Library/ViewModels/DiscoveryPostcardViewModel.swift

+  var metadataIcon: Signal<UIImage?, NoError> { get }
+
+  /// Emits metadata icon and text color
+  var metadataIconAndTextColor: Signal<UIColor, NoError> { get }


Just checking, this only seems to be used for the textColor, is metadataIconAndTextColor the right name?

bormind · 2017-07-31T15:54:21Z

Thank you @justinswart!

bormind · 2017-08-01T23:52:06Z

Thank you @justinswart, @theginbin !!

bormind added 4 commits July 12, 2017 16:07

Removed heart icon from Project of the day label

eb636ba

Test project postcard view controller for featured and potd projects

1d4ac9f

removed whitespace

4808368

replaced metadata icon in dicovery poscard in the storyboard

b00aaf3

bormind requested a review from theginbin July 13, 2017 20:25

theginbin suggested changes Jul 13, 2017

View reviewed changes

Removed not used metadata-podt icon from assets

94d413a

Fixed shifted meta label if metahIcon is not present

8e04343

justinswart reviewed Jul 14, 2017

View reviewed changes

theginbin approved these changes Jul 14, 2017

View reviewed changes

bormind added 2 commits July 20, 2017 08:19

Separate signal for hiding metadata icon

2d8bd22

Merge branch 'master' into proj-of-the-day-label

119ed64

bormind added 4 commits July 28, 2017 12:00

Created signal for metadata properties

227c4b6

Merge branch 'master' into proj-of-the-day-label

9a468ee

Tests for metadata signals

1673701

Cleanup

79cb778

justinswart reviewed Jul 31, 2017

View reviewed changes

Fixed naming and spelling mistakes

f97ea20

bormind merged commit 743f999 into master Aug 1, 2017

justinswart deleted the proj-of-the-day-label branch August 13, 2019 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proj of the day label #205

Proj of the day label #205

bormind commented Jul 13, 2017 •

edited

theginbin left a comment

theginbin Jul 13, 2017

bormind Jul 13, 2017

theginbin Jul 13, 2017

bormind commented Jul 13, 2017

bormind commented Jul 13, 2017

justinswart left a comment

justinswart Jul 14, 2017

bormind Jul 14, 2017

theginbin Jul 14, 2017

bormind Jul 14, 2017 •

edited

theginbin Jul 14, 2017

theginbin commented Jul 14, 2017

bormind commented Jul 14, 2017

theginbin commented Jul 14, 2017

justinswart commented Jul 17, 2017

justinswart commented Jul 24, 2017

justinswart commented Jul 26, 2017

justinswart left a comment

justinswart Jul 31, 2017

justinswart Jul 31, 2017

justinswart Jul 31, 2017

bormind commented Jul 31, 2017

bormind commented Aug 1, 2017

Proj of the day label #205

Proj of the day label #205

Conversation

bormind commented Jul 13, 2017 • edited

What

Why

How

See 👀

theginbin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bormind commented Jul 13, 2017

bormind commented Jul 13, 2017

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bormind Jul 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theginbin commented Jul 14, 2017

bormind commented Jul 14, 2017

theginbin commented Jul 14, 2017

justinswart commented Jul 17, 2017

justinswart commented Jul 24, 2017

justinswart commented Jul 26, 2017

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bormind commented Jul 31, 2017

bormind commented Aug 1, 2017

bormind commented Jul 13, 2017 •

edited

bormind Jul 14, 2017 •

edited