Skip to content
This repository was archived by the owner on Sep 20, 2023. It is now read-only.

Feature/show issues and pr count #2767

Merged
merged 6 commits into from
Jun 10, 2019

Conversation

LamourBt
Copy link
Contributor

@LamourBt LamourBt commented Jun 3, 2019

This feature is based on this ticket #2718

Copy link
Member

@jdisho jdisho left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @LamourBt
Couple of comments from my side.

Copy link
Collaborator

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

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

This is looking good, thank you so much @LamourBt!

Some small things, and we can get this in 😎

Copy link
Collaborator

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

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

🤩

Copy link
Collaborator

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

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

Ah, much better, lovely! Thanks so much for your patience again, @LamourBt 💪

return NSLocalizedString(newTitle, comment: "")
}

func composeLocalizedTitle() -> String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could combine these two functions, keeping the logic in the function rather than the caller?
To me that seems to make the code a little easier to grasp, and putting all the logic in the function.

// set the title
title = composeLocalizedTitle(using: itemCount)

func composeLocalizedTitle(using itemCount: Int?) -> String {
    let newTitle: String
    if let itemCount = itemCount, itemCount > 0 {
        switch type {
        case .issues: newTitle = "Issues (\(itemCount))"
        case .pullRequests: newTitle = "Pull Requests (\(itemCount))"
        }
    } else {
        switch type {
        case .issues: newTitle = "Issues"
        case .pullRequests: newTitle = "Pull Requests"
        }
    }
    return NSLocalizedString(newTitle, comment: "")
}

What do you think?

@@ -157,14 +162,23 @@ EmptyViewDelegate {

self.controllers = controllers
}

func buildQueryString(using repo: RepositoryDetails, _ searchString: String) -> String {
return "repo:\(repo.owner)/\(repo.name) \(searchString) ".lowercased()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be two spaces ( ) between the name and the string?

@BasThomas
Copy link
Collaborator

Merging; we can address the two comments later if we want to. They are no blocker for this awesomeness!

@BasThomas BasThomas merged commit 97a8fbb into GitHawkApp:master Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants