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

[MBL-1495] [MBL-1502] PPO UI implementation #2186

Merged
merged 19 commits into from
Nov 1, 2024

Conversation

stevestreza-ksr
Copy link
Contributor

📲 What

This PR connects all the individual pieces for PPO cards to the PPO view, populating the list's various states (empty, error, loaded, refreshing, etc), and connecting the actions to their relevant view model calls so that they bubble up properly.

It also implements the error state view, since it wasn't terribly complex.

🛠 How

All the individual pieces were added in previous PRs, so this PR just unifies everything.

👀 See

image

Figma

♿️ Accessibility

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

✅ Acceptance criteria

Enable the Pledged Projects Overview feature flag on a user that has it enabled, and navigate to the Activity tab. You should see the Project Alerts load and appear, and be functional.

Tapping buttons or navigating places doesn't work yet, that will complete with other PRs for MBL-1451.

@nativeksr
Copy link
Collaborator

nativeksr commented Oct 30, 2024

1 Warning
⚠️ Big PR

SwiftFormat found issues:

File Rules
Kickstarter-iOS/Features/PledgedProjectsOverview/CardView/PPOProjectCardModel.swift:399:1 warning: (wrap) Wrap lines that exceed the specified maximum width.
Kickstarter-iOS/Features/PledgedProjectsOverview/CardView/PPOProjectCardViewModel.swift:64:1 warning: (wrap) Wrap lines that exceed the specified maximum width.

Generated by 🚫 Danger

@stevestreza-ksr stevestreza-ksr force-pushed the stevestreza/ppo/ui-implementation branch from d0f43ba to bdd1c07 Compare October 30, 2024 18:03
@stevestreza-ksr stevestreza-ksr marked this pull request as ready for review October 31, 2024 01:28
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

I feel pretty out of the loop when it comes to the individual pieces being wired up, but this code looks good to me. I left a few comments that are mostly for my own education lol.

Looking at the designs though I'm seeing some slight differences in the main action button style. The button corners in the designs are less rounded than the screen shot here and also have more of a box shadow. That might be out of date or something you're doing later. Just calling out as I see things.

@stevestreza-ksr
Copy link
Contributor Author

I think you're looking at the Android designs, not the iOS designs which don't have drop shadows. I'll do another look at it though, as the iOS Figma does seem to have changed a little since original implementation.

Copy link
Contributor

@ifosli ifosli left a comment

Choose a reason for hiding this comment

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

Overall, really nice! Very exciting! There is one unfortunate merge conflict - the viewBackingDetails action I'm adding is the same as the one you're calling showProject. I think calling it showProject is unfortunately misleading when Alison wants it to open the backing details webview, but I do wish that it was opening the project page instead.

I'm going to mark this as approved, though I do think this should be renamed before merging (and I still think it makes more sense to just have one onPerformAction handler instead of multiple), since those changes should be very straightforward. But lmk if you want me to take another pass at this!

@stevestreza-ksr stevestreza-ksr merged commit 2534a52 into main Nov 1, 2024
5 checks passed
@stevestreza-ksr stevestreza-ksr deleted the stevestreza/ppo/ui-implementation branch November 1, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants