User can only have 2 initials in UI #544
Conversation
…rns a max of two. If there are more than two, keep the first and last initial and drop any in the middle.
|
I'm looking to make a 'good' PR. So go over with a fine tuned comb and shout. I already see a couple things I would like to have done better. Not sure if the function should upcase for example. |
Gemfile.lock
Outdated
| @@ -437,6 +437,7 @@ GEM | |||
|
|
|||
| PLATFORMS | |||
| arm64-darwin-23 | |||
| arm64-darwin-24 | |||
There was a problem hiding this comment.
I don’t know what this line is doing, do you? I’m not opposed to adding it if it’s helping something, but if it’s just a stray line we should remove it.
There was a problem hiding this comment.
That was added automatically. I just assumed cause I'm on a mac m1. I can take it out ... maybe :) I'll look into git-ing it out.
There was a problem hiding this comment.
I removed the arm64-darwin-24. I think lol. Thanks for your patience.
app/views/layouts/_user_avatar.erb
Outdated
| @@ -11,5 +11,5 @@ | |||
| <%= classes %> | |||
| " | |||
| > | |||
| <%= user.name.initials&.upcase %> | |||
| <%= only_user_initials(user.name.initials).upcase %> | |||
There was a problem hiding this comment.
The method name is a bit confusing. I find myself thinking, “but isn’t name.initialis” only the user initials? I don’t expect that to return something other than initials.
A better name might be: “only_two_initials”
There was a problem hiding this comment.
only_two_initials makes great sense
app/helpers/application_helper.rb
Outdated
| @@ -1,4 +1,21 @@ | |||
| module ApplicationHelper | |||
|
|
|||
| def only_user_initials(initials, limit: 2) | |||
| letters = initials.to_s.downcase.strip.split("") | |||
There was a problem hiding this comment.
I suspect this whole method can be a lot simpler. Probably this will work:
def …
return initials if initials.nil? || initials.length <= 2
initials[0] + initials[-1]
end
I haven’t tested that so I may be overlooking something, but I think that’s a simple way to grab the first letter and the last letter.
app/views/layouts/_user_avatar.erb
Outdated
| @@ -11,5 +11,5 @@ | |||
| <%= classes %> | |||
| " | |||
| > | |||
| <%= user.name.initials&.upcase %> | |||
| <%= only_user_initials(user.name.initials).upcase %> | |||
There was a problem hiding this comment.
Also, I think I’d keep the &.upcase rather than removing &, at least the method the way I wrote it returns a nil if it’s passed a nil, which seems like reasonable behavior
|
|
||
| test "only at most 2 initials with default limit" do | ||
| assert_equal "jz", only_user_initials("jdz") | ||
| assert_equal "pl", only_user_initials("Plllll") |
There was a problem hiding this comment.
It’s a bit unexpected that this method would change case. The way I wrote it preserves whatever case was passed in. I think that’s what most users would expect.
Also, this second test is bad because the fact that it returns “pl” does not confirm that it’s returning the first letter and the last letter. There are a lot of “l”s
There was a problem hiding this comment.
No longer changing case. Took out a few l's.
| test "can have spaces" do | ||
| assert_equal "pq", only_user_initials("P vv qQ") | ||
| end | ||
|
|
There was a problem hiding this comment.
It’s worth adding a test for the nil case
|
Ok, I added those awesome/needed/helpful changes. I appreciate you doing that. |
|
DAMN! Did no mean to CLOSE the PR. |
|
These code versioning tools are awesome but I feel like I'm running with scissors. I might have done something wrong - but I learn. Really, make any other observations. |
|
I appreciate your patience and your coding skills. You can delete. I'll do better on the next one. |
Addresses: "You should create a method that takes user.name.initials as its argument. We are already properly extracting initials, we just want fewer of them. So make a helper like you’ve done that takes in all initials and returns a max of two. If there are more than two, keep the first and last initial and drop any in the middle."
I think this kind of works - but I've not done this type of process in a while so I expect it is not perfect ... yet.