status-im / status-protocol-go

Status Protocol implementation in Go
Mozilla Public License 2.0
0 stars 1 forks source link

Port identicon & gfycat to status #74

Closed cammellos closed 4 years ago

cammellos commented 4 years ago

Adds two fields, generatedName ( I will rename to gfycat maybe? ) and Identicon

code for identicon taken from https://github.com/jgimeno/go-identicon , slightly modified

cammellos commented 4 years ago

let's make sure we handle errors

Done

let's add comments at least for exported methods

Done

why is it called gfycat? :D

I think gfycat popularized it https://medium.com/@Gfycat/naming-conventions-97960fc40179 , not sure what a better name would be

let's make sure we export only what's needed so that we don't need to maintain huge public interface

Done

jakubgs commented 4 years ago

not sure what a better name would be

Agree with Adam. Names that require obscure context are bad. Something generic like identity, and put the icon module in there too under icon, so you get identity/icon and identity/username or something like that.

cammellos commented 4 years ago

@adambabik @jakubgs I have moved: identity/username -> 3 random words identity/identicon -> Picture

And the corresponding fields: username and identicon in contact.

There some potential confusion with name which is set by the user, so it kind of a username as well, but I think we are getting rid of it eventually, so should not be a problem. Let me know what you think

jakubgs commented 4 years ago

There some potential confusion with name which is set by the user, so it kind of a username as well

That's a good point. I guess it is a bit close. Maybe pseudonim or alias would be closer to the menaing?

cammellos commented 4 years ago

@jakubgs alias sounds reasonable, I might change it to that if there's not any objections, @adambabik ?

jakubgs commented 4 years ago

Is this ready to merge or is there anything else to be done here @adambabik ?

cammellos commented 4 years ago

@jakubgs it needs to go through testing in status-react, I'd rather get a green light there and then merge, otherwise if there's anything to fix I will have to open multiple prs etc

jakubgs commented 4 years ago

Yeah, makes sense, just wanted to know what's left to be done, thanks!