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

Issue 32 add credits screen #37

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

christopher-fredregill
Copy link

@cardoso care to take a look? Not sure if this is exactly what you had in mind, but I tried to adhere to the requirements you laid out in #32 .

Also, not sure how you'd like this screen to be accessed from within the app. I temporarily added this line in AppDelegate.swift just to toy around with it locally:

window?.rootViewController = ContributorListController(rootView: ContributorList())

Here are some sample screenshots:
Simulator Screen Shot - iPhone 11 - ContributorList View
Simulator Screen Shot - iPhone 11 - ContributorProfile View

Thanks!

@cardoso
Copy link
Owner

cardoso commented Oct 14, 2019

Hi @christopher-fredregill . Thanks for your contribution!

I think the UI should adhere to the current style (black/dark). See the screenshots in the README.
The screen should be accessible from a button on the left side of the navigation bar of the first screen. Use the “gear” symbol from SFSymbols.

I will take a better look at your PR after these changes! Thank you again! 🙏

@cardoso cardoso self-requested a review October 14, 2019 14:55
@cardoso cardoso added hacktoberfest 🎉 feature New feature or request labels Oct 14, 2019
Copy link
Owner

@cardoso cardoso left a comment

Choose a reason for hiding this comment

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

Changes requested in comment above

@christopher-fredregill
Copy link
Author

@cardoso is this where the gear button should go?

Simulator Screen Shot - iPhone 11 - 2019-10-14 at 21 10 19

@codecov-io
Copy link

Codecov Report

Merging #37 into master will decrease coverage by 4%.
The diff coverage is 31.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
- Coverage    70.2%   66.19%   -4.01%     
==========================================
  Files          13       17       +4     
  Lines         443      494      +51     
==========================================
+ Hits          311      327      +16     
- Misses        132      167      +35
Impacted Files Coverage Δ
...xMovieDB/Controllers/MovieListViewController.swift 89.58% <ø> (ø) ⬆️
ReduxMovieDB/Views/ContributorList.swift 0% <0%> (ø)
ReduxMovieDB/Model/Contributor.swift 0% <0%> (ø)
...ovieDB/Controllers/ContributorListController.swift 0% <0%> (ø)
ReduxMovieDB/Views/ContributorProfile.swift 0% <0%> (ø)
ReduxMovieDB/AppDelegate.swift 100% <100%> (ø) ⬆️
ReduxMovieDB/State/MainState.swift 100% <100%> (ø) ⬆️
ReduxMovieDB/Thunks.swift 64.17% <91.66%> (+5.99%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f57d4cb...75908b8. Read the comment docs.

@cardoso
Copy link
Owner

cardoso commented Oct 18, 2019

@christopher-fredregill yes! Sorry for the late response

@squash-labs
Copy link

squash-labs bot commented Jan 17, 2023

Manage this branch in Squash

Test this branch here: https://issue-32-add-credits-screen-r73y9.squash.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 feature New feature or request hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants