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

Luko/offer req card user profile #180

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

lucianoschillagi
Copy link
Contributor

@lucianoschillagi lucianoschillagi commented Jun 30, 2020

What this PR does:
Creates Offer Request Card User Profile

Checklist

  • Compiler warnings resolved
  • Linter warnings resolved
  • User-facing strings in Localizable.strings file
  • Unused code, print statements, and comments removed

Which issue(s) this PR fixes:

Resolves #171 #172 #173

Special notes for reviewers:
I had to limit the OfferRequestCard initializer (SwiftLint requirements - 300 characters max)

Additional comments:

@lucianoschillagi
Copy link
Contributor Author

Screen Shot 2020-06-29 at 11 32 51 PM

Comment on lines 29 to 48
class OfferRequestCard: UIStackView {

var headerStack = HeaderOfferRequestCard(postedDate: "Posted 14hrs ago",
offer: "Offers",
supplyType: "Medical Supplies")

var subHeaderStack = SubHeaderOfferRequestCard(initials: "AM",
name: "Ana Muller",
location: "Berlin, Germany")

var bodyStack = BodyOfferRequestCard(tit: "I have 100 face Masks to give away",
message: "I have a small store and I would like to give away 100 mask but I don’t know where, please contact me if you need them. I have a small store and I would like to give away 100 mask but I don’t know where, please contact me if you need them. please contact m ...")

var footerStack = FooterOfferRequestCard(numOfLikes: 4,
numOfComments: 7)

override init(frame _: CGRect) {
super.init(frame: .zero)
}

Copy link
Contributor

@ethanswift ethanswift Jun 30, 2020

Choose a reason for hiding this comment

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

Thanks, nice work. only one point.

Is this the final class for the feed? if so can you also provide an initializer that takes all the required parameters (title body text, offer/request, ....), so that we can use them as a card in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I am going to fix that and then I will send the PR again 🤙

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lucianoschillagi
Copy link
Contributor Author

Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

https://jmp.sh/hzBWmgt

the tag is smaller on height on design
view more text is smaller on design
numbers near the heart icon/share icon/etc on design are smaller and are grey instead of black

Great, I am going to fix that. Thanks.

Copy link
Contributor

@ethanswift ethanswift left a comment

Choose a reason for hiding this comment

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

Thanks, good work.

Please don't forget to make the changes that was requested (by others), before merging

@lucianoschillagi
Copy link
Contributor Author

@stavares843 the new PR is ready

Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

https://jmp.sh/C7ffyDI

  • blue dot is not the same color as the design, also is less rounder on the app
  • on design theres less spacing between the view more copy and the icons section
  • seems theres less spacing between the numbers and the icons on the design compared with the app

@lucianoschillagi
Copy link
Contributor Author

Great, tomorrow I will send the new PR, thanks!

let feedOfferRequestTime = FeedOfferReqTime(offerReq: .offers, timeStamp: "Posted 14hrs Ago")
let feedCategoryTag = FeedCatTag(categoryTagTxt: "Medical Supplies")

init(postedDate _: String, reqOrOffer _: String, supplyType _: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The parameters in this initializer seem redundant, any special reason why we have them here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I am going to work on this issue tomorrow.
Thanks a lot for your comments @OnyekachiSamuel !

// The Offer Request Card (Feed)
let offerReqCard = OfferRequestCard(headerData: .init(postedDate: "P", reqOrOffer: "O", supplyType: "M"), subHeaderData: .init(initials: "AM", name: "Ana Muller", location: "Berlin, Germany"), bodyData: .init(tit: "I", message: "E"), footerData: .init(numOfLikes: 4, numOfComments: 7))

let postCardTableView = UITableView()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This tableView appears not used, we need to populate this tableView with list of offers, requests, or all as the case maybe. Let me know if you don't understand so that I can explain more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I understand.
But I think that this implementation maybe should be in another ticket. What do you think?

@OnyekachiSamuel
Copy link
Collaborator

OnyekachiSamuel commented Jul 6, 2020

We have lots of broken constraint warnings, please can you help resolve them, thanks.

I don't see this warnings in my Xcode (see screenshot please)
Screen Shot 2020-07-07 at 10 06 42 AM

.top(to: \.topAnchor, constant: 20)
.left(to: \.leftAnchor, constant: 20)
.right(to: \.rightAnchor, constant: -20)
.width(UIScreen.main.bounds.width)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't actually need this width constraint since you have already constrained the view to the left and right by some margin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have remove it.

@OnyekachiSamuel
Copy link
Collaborator

OnyekachiSamuel commented Jul 7, 2020

We have lots of broken constraint warnings, please can you help resolve them, thanks.

I don't see this warnings in my Xcode (see screenshot please)
Screen Shot 2020-07-07 at 10 06 42 AM

Check the console output on Xcode, you will see the broken constraints.

Yes! Right now I see the restrictions broken. A lot of them! I need your help to solve this. Can you help me? May we have a Google meeting? Friday Saturday...? What do you think?

@devarshjoshi
Copy link
Collaborator

@lucianoschillagi @OnyekachiSamuel would appreciate it if you guys can meetup and resolve this.

@lucianoschillagi
Copy link
Contributor Author

lucianoschillagi commented Jul 11, 2020

Yes, I need some help to resolve this. I am available for a meet... @OnyekachiSamuel are you available some of this days? Thanks

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.

Feed/Offer-Request Card Layout
5 participants