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-1238] Adding location tag to projects sorted by distance #1186

Merged
merged 9 commits into from
May 7, 2020

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented May 6, 2020

📲 What

Adds the location tag on project cards if projects have been sorted by distance.

🤔 Why

For the Lights On feature, we'd like to show users projects near them. We'll be accomplishing this by using the .distance sort on the v1 API. We would also like to show users the location of the project on the project cards if they're seeing projects that are sorted by distance.

🛠 How

Adding the location tag was pretty trivial in itself, but positioning it correctly turned out to be difficult given the convoluted and sub-optimal hierarchy of elements in the project cards. I decided to re-organize all the subviews in the project cards into a more manageable and simple heirarchy. This resulted in:

  • all layout conflict console warnings are now resolved
  • the "You're a backer" metadata view now shows the icon (this has been broken due to an incorrect constraint for a long long time)
  • the padding around the project info and all it's sub-elements is now consistent
  • screenshot tests for the "Featured Project" feature are now fixed
  • screenshot tests have been added for the social facepile, and the "You're a backer" state
  • dynamic type is now working slightly better in the project cards

The location tag will only show if the projects have been sorted by distance.

👀 See

Please note that the after screenshot is showing the "You're a backer" and location tags programatically.

After 🦋 Before 🐛
Simulator Screen Shot - iPhone 8 - 2020-05-05 at 18 22 02 Simulator Screen Shot - iPhone 8 - 2020-05-06 at 11 31 09

♿️ Accessibility

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

✅ Acceptance criteria

Replace Line 278 in DiscoveryPostcardViewModel with the following code to show the location tag:

    self.locationStackViewHidden = params.map { params in
        return false
      // guard let params = params else { return false }

      // return params.sort != .distance
    }
  • launch the app and examine the project cards. The location tag should show, and the layout should match the designs. There should be no console layout warnings.

@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Do we know if we want the category name and PWL to also resize with a11y?

image

@@ -6210,7 +6210,7 @@
GCC_WARN_UNUSED_FUNCTION = YES;
GCC_WARN_UNUSED_VARIABLE = YES;
IPHONEOS_DEPLOYMENT_TARGET = 12.4;
MARKETING_VERSION = 4.5.1;
MARKETING_VERSION = 4.6.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple of mismatched version numbers in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! reverting

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Great job fixing those layout warnings! Now I'm just seeing one from DiscoveryNavigationHeaderViewController which I think should be easy enough to silence.

@ifbarrera
Copy link
Contributor Author

Do we know if we want the category name and PWL to also resize with a11y?

image

The tags weren't originally built with accessibility support, so I didn't want to go down the rabbit hole of adding it now since we're likely to rebuild these cards.

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Nice!

@ifbarrera ifbarrera merged commit 407b02f into master May 7, 2020
@ifbarrera ifbarrera deleted the NT-1238-location-tag branch May 7, 2020 17:39
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

3 participants