-
-
Notifications
You must be signed in to change notification settings - Fork 71
Improve and shorten wordings in right panel of profile #1935
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
Improve and shorten wordings in right panel of profile #1935
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
app/views/users/show.html.erb
Outdated
| <% if current_user&.id == @user.id %> | ||
| <%= link_to search_path(search: "user:#{@user.id} post_type:2"), 'aria-label': 'View your answers' do %> | ||
| Answers | ||
| <% end %> | ||
| <% else %> | ||
| <%= link_to search_path(search: "user:#{@user.id} post_type:2"), 'aria-label': "View answers from #{rtl_safe_username(@user)}" do %> | ||
| Answers | ||
| <% end %> | ||
| <% end %> | ||
| </td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all these widgets only differ in what aria-label (and sometimes what link param is used), let's make these less verbose. We also have a convenience same_as? method on User that is both nil-safe and easier to read than current_user&.id == @user.id (@user.same_as?(current_user)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something along the lines of (where is_me can be shared with all the other widgets):
<% is_me = @user.same_as?(current_user) %>
<%= link_to search_path(search: "user:#{@user.id} post_type:2"),
'aria-label': is_me ? 'View your answers' : "View answers from #{rtl_safe_username(@user)}" do %>
Answers
<% end %>(somehow GitHub's editor got even worse, so apologies for the single-line aria-label)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd agree with this - these feel like they're missing some ternaries or helper methods, rather than full if statements for a relatively simple wording change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I've applied this change so the new code and a few existing places in the same file now use is_me. Rather than split a ternary across multiple lines I've put the value for 'aria-label:' on the next line down from the property name (for the one line that exceeded 120 characters otherwise). Let me know if there's a preferred way to split long lines for ERB files rather than this.
This comment was marked as resolved.
This comment was marked as resolved.
Oaphi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested the updated version out - LGTM
The changes suggested in meta:287189:
Also:
Notice that the height of the panel is now significantly reduced.
Appearance before (left) and after (right)