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

Redesign Licenses Screen #31

Closed
wants to merge 3 commits into from
Closed

Conversation

kevinrpb
Copy link

@kevinrpb kevinrpb commented Oct 2, 2019

I thought the TextView looked a bit odd with the rest of the design. I'm starting to refactor it a bit. So far I've put the licenses into a JSON (this also allows to manage them dynamically if they change; or even fetch them if appropriate). Using this, the Licenses navigation gets you to a table view that will display a line for each product.

Here's a preview of the new screen

@kevinrpb kevinrpb force-pushed the master branch 2 times, most recently from 7953c64 to fae4610 Compare October 7, 2019 10:25
@kevinrpb kevinrpb marked this pull request as ready for review October 7, 2019 17:36
@kevinrpb
Copy link
Author

kevinrpb commented Nov 4, 2019

@rileytestut any chance for feedback here when you have some time? =)

@dlevi309
Copy link

dlevi309 commented Nov 6, 2019

@kevinrpb I just saw this preview and I actually really enjoyed it (not to sound surprised, all i mean by that is "how can you improve the licensing screen?" was my expectation) but I really like it, are you a part of the discord? if you'd like i could forward this over to him.

@kevinrpb
Copy link
Author

kevinrpb commented Nov 6, 2019

@dlevi309 Thank you =) I am not part of it. Is it public?

@dlevi309
Copy link

dlevi309 commented Nov 6, 2019

@kevinrpb yeah man :) and it is not public, you would have to join the 5$ a month pledge on his patreon, he's very responsive on it and would probably love to see this directly from you, but id be happy to put this on the channel for you 🙂

}

navigationItem.title = product
copyrightLabel.text = copyright != "" ? copyright : "(no copyright line)"

Choose a reason for hiding this comment

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

Can we use copyright.isEmpty?


let url = URL(fileURLWithPath: path)

guard let data = try? Data(contentsOf: url), let json = try? JSONSerialization.jsonObject(with: data) as? [[String: String]] else {

Choose a reason for hiding this comment

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

I suggest to go with Codable protocol instead of JSONSerialization

break
default:
cell.style = .middle
}

Choose a reason for hiding this comment

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

Move this switch case comparison to separate function. Like
cell.style = getStyleFor(license: [String: String])

@OscarGorog
Copy link

Would be great to get this approved, looks great!

@HPaulson
Copy link

HPaulson commented Apr 4, 2020

Looks amazing! +1

@kevinrpb kevinrpb changed the base branch from master to develop April 4, 2020 13:15
@kevinrpb
Copy link
Author

kevinrpb commented Apr 4, 2020

I'm closing this PR in favor of #149 since I have updated my fork to follow Git Flow and moved the changes to a new branch. I also took @ivar1803 suggestions and applied them.

@kevinrpb kevinrpb closed this Apr 4, 2020
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