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 #149

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

Conversation

kevinrpb
Copy link

@kevinrpb kevinrpb commented Apr 4, 2020

(originally on #31):

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

I moved the PR because I updated the branch to follow Git Flow as the main repo.

I also accepted the suggestions made there.

# Conflicts:
#	AltStore.xcodeproj/project.pbxproj
#	AltStore/Settings/LicensesViewController.swift
#	AltStore/Settings/Settings.storyboard
# Conflicts:
#	AltStore/Settings/Settings.storyboard
@kevinrpb kevinrpb mentioned this pull request Apr 4, 2020
switch indexPath.row {
case 0:
cell.style = .top
break

Choose a reason for hiding this comment

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

We don't need to declare break. Please remove it.

break
case licenses.count - 1:
cell.style = .bottom
break

Choose a reason for hiding this comment

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

We don't need to declare break. Please remove it.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the review. You're right hehe.

Right now the PR can't be merged and since it has been opened for months without answer from Riley I'm not in a rush to update it. I will get to updating it eventually though and I will take your recommendation into account 😄

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