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

Conversation

@LucianoPAlmeida
Copy link
Collaborator

Hey guys :))

This is a simple improvement on the IssueRequest changing the message when the users are the same. The same message that appears on the GitHub web interface.
From:
simulator screen shot - iphone x - 2018-12-24 at 12 22 44
To:
simulator screen shot - iphone x - 2018-12-24 at 12 23 39

Also, some warnings removed :)

Copy link
Collaborator

@Huddie Huddie left a comment

Choose a reason for hiding this comment

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

Cool stuff! Some nits.


let styledString: StyledTextString
if actor == user {
styledString = IssueRequestModel.buildSelfString(actor: actor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: newline this like

buildSelfString(
       actor: actor,
       ...
)

date: date,
event: event)
} else {
styledString = IssueRequestModel.buildString(actor: actor,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

).warm(width: width)
}

static private func buildSelfString(actor: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

newline this (same as above)

function(
    param1: ...,
    param2 ...,
) -> return {

return builder.build()
}

static private func buildString(actor: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

}

static private func buildSelfString(actor: String,
user: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

user is never used inside this function so no need to pass it in. Instead of passing in actor you may want to pass in user and switch the use of actor later on to user. I think it reads better as user


let image = UIImage(named: "chevron-down").withRenderingMode(.alwaysTemplate)
let optionButtonWidth = (image.size.width ?? 0) + (2 * Styles.Sizes.gutter)
let optionButtonWidth = image.size.width + (2 * Styles.Sizes.gutter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't relevant to this PR

event: event)
}

self.string = StyledTextRenderer(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: You don't need self. here right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't need, but I think is better for consistency since all other members being initialized require self. So doing all members even this with it :))

Copy link
Collaborator

Choose a reason for hiding this comment

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

They need self. because the parameter names match the variable names

Copy link
Collaborator Author

@LucianoPAlmeida LucianoPAlmeida Dec 24, 2018

Choose a reason for hiding this comment

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

Yeah, but what I mean by consistency is

        self.id = id
        self.actor = actor
        self.user = user
        self.date = date
        self.event = event
        string = ... // Also a member 
        // Even it not needing, to maintain consistency in all memeber initialization 
       self.string = ... 

But can change if it is a problem :)

@Huddie Huddie added the 💤 awaiting review Pull Request is awaiting code reviews label Dec 24, 2018
@rnystrom rnystrom merged commit 75de7aa into GitHawkApp:master Dec 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

💤 awaiting review Pull Request is awaiting code reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants