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.

Sign up for GitHub

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

Jump to bottom

[UI] Modify datasource and layout on DonationsMoreDetails controller. #134

Merged
merged 16 commits into from Jun 9, 2020
Merged

[UI] Modify datasource and layout on DonationsMoreDetails controller. #134

merged 16 commits into from Jun 9, 2020

Conversation

dasautoooo
Copy link
Member

@dasautoooo dasautoooo commented Jun 8, 2020 •
edited

Changes:

  • Change donation detail's image source to donation's bannerImagePath.
  • Remove donation detail's outcome section.
  • Add DonationTypes to determine which image should be displayed on DonationsMoreDetials VC.
  • Remove the first donate button.
  • Change the sticky donate button title.

Screenshots:

Simulator Screen Shot2
Simulator Screen Shot1

@dasautoooo dasautoooo requested a review from a team June 8, 2020 12:33
@dasautoooo dasautoooo added good first issue Good for newcomers review me This PR needs more reviewers labels Jun 8, 2020
@hybridcattt
Copy link
Member

What is the context for removing the section? Is it because it's excluded from v1?

malcolmkmd
malcolmkmd previously approved these changes Jun 8, 2020
View reviewed changes
Copy link

@malcolmkmd malcolmkmd left a comment

Choose a reason for hiding this comment

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

LGTM

@dasautoooo
Copy link
Member Author

What is the context for removing the section? Is it because it's excluded from v1?

Because there's no data support this section, it is always empty. We can add this back if the data modal needs it.

hybridcattt
hybridcattt reviewed Jun 8, 2020
View reviewed changes
Copy link
Member

@hybridcattt hybridcattt left a comment

Choose a reason for hiding this comment

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

It's preferred to use FeatureFlags to disable unfinished features, it's been discussed and decided in previous PRs (got that feedback myself)

@dasautoooo
Copy link
Member Author

dasautoooo commented Jun 8, 2020 •
edited

What is the context for removing the section? Is it because it's excluded from v1?

Because there's no data support this section, it is always empty. We can add this back if the data modal needs it.

The API data has this outcome key, but the value for the key is null for all data.
The data model for Donation doesn't have outcome.

let id: Int
let title: String
let description: String
let link: String
let person: Person?
let type: DonationType?
let bannerImagePath: String?

@dasautoooo dasautoooo changed the title [CHORE] Modify datasource and layout on DonationsMoreDetails controller. [FEATURE] Modify datasource and layout on DonationsMoreDetails controller. Jun 8, 2020
@dasautoooo dasautoooo changed the title [FEATURE] Modify datasource and layout on DonationsMoreDetails controller. [UI] Modify datasource and layout on DonationsMoreDetails controller. Jun 8, 2020
@dasautoooo
Copy link
Member Author

it's now using FeatureFlags to disable outcome section.

hybridcattt
hybridcattt reviewed Jun 8, 2020
View reviewed changes
Copy link
Member

@hybridcattt hybridcattt left a comment

Choose a reason for hiding this comment

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

I've looked a bit closer and have couple more questions

...heirNames/Source/Controller/Donations/DonationsMoreDetails/DMDCollectionViewDataSource.swift Outdated Show resolved Hide resolved
...Source/Controller/Donations/DonationsMoreDetails/DonationsMoreDetailsController+Layout.swift Outdated Show resolved Hide resolved
@dasautoooo dasautoooo requested a review from a team June 8, 2020 15:59
Kilo-Loco
Kilo-Loco previously approved these changes Jun 8, 2020
View reviewed changes
Copy link
Member

@Kilo-Loco Kilo-Loco left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@Kilo-Loco Kilo-Loco added the branch-out-of-date The branch is out of date with development. label Jun 8, 2020
@dasautoooo dasautoooo removed branch-out-of-date The branch is out of date with development. good first issue Good for newcomers labels Jun 8, 2020
@dasautoooo dasautoooo requested review from Sharkesm and a team June 9, 2020 03:46
ohwhen
ohwhen previously approved these changes Jun 9, 2020
View reviewed changes
Copy link
Collaborator

@ohwhen ohwhen left a comment

Choose a reason for hiding this comment

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

🔥

Sharkesm
Sharkesm previously approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@Sharkesm Sharkesm left a comment

Choose a reason for hiding this comment

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

⭐

@dasautoooo
Copy link
Member Author

dasautoooo commented Jun 9, 2020 •
edited

🚨Do NOT merge right now. It needs some changes.

@dasautoooo dasautoooo added being worked on Work is in progress, more changes are coming and removed review me This PR needs more reviewers labels Jun 9, 2020
@dasautoooo dasautoooo dismissed stale reviews from bertadevant and Sharkesm via 3bcf05a June 9, 2020 09:24
@dasautoooo dasautoooo added review me This PR needs more reviewers and removed being worked on Work is in progress, more changes are coming labels Jun 9, 2020
@dasautoooo
Copy link
Member Author

dasautoooo commented Jun 9, 2020 •
edited

🚨 Do NOT merge right now. It needs some changes.

The alarm has been lifted 😉.

Sharkesm
Sharkesm previously approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@Sharkesm Sharkesm left a comment

Choose a reason for hiding this comment

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

Overall looks good 👍

SayTheirNames/Source/Controller/Donations/DonationsMoreDetails/DonationTypes.swift Outdated
// }
// ]
//}

Copy link
Member

Choose a reason for hiding this comment

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

@dasautoooo Can you remove these comments if they don't represent anything

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this intentionally because I wanna make sure other people know why this should start from 1.

SayTheirNames/Source/Controller/Donations/DonationsMoreDetails/DonationTypes.swift Show resolved Hide resolved
bertadevant
bertadevant previously approved these changes Jun 9, 2020
View reviewed changes
@dasautoooo dasautoooo dismissed stale reviews from bertadevant and Sharkesm via f085767 June 9, 2020 09:52
@Sharkesm Sharkesm merged commit 4f4458c into development Jun 9, 2020
@Sharkesm Sharkesm deleted the chore/change-donation-detail-layout branch June 9, 2020 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malcolmkmd malcolmkmd malcolmkmd left review comments

@bertadevant bertadevant bertadevant approved these changes

@Sharkesm Sharkesm Sharkesm approved these changes

@ohwhen ohwhen ohwhen left review comments

@Kilo-Loco Kilo-Loco Awaiting requested review from Kilo-Loco

@hybridcattt hybridcattt Awaiting requested review from hybridcattt

Assignees
No one assigned
Labels
review me This PR needs more reviewers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dasautoooo @hybridcattt @ohwhen @bertadevant @Kilo-Loco @Sharkesm @malcolmkmd