-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: Add profile option to always view full names in event view #566
base: master
Are you sure you want to change the base?
feat: Add profile option to always view full names in event view #566
Conversation
Created migration to add :prefers_full_name to member. Added the option to edit it. Made it change the way names are displayed by event_role.
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.
looks pretty solid! I have some thoughts on the naming choices but implementation lgtm!
app/models/event_role.rb
Outdated
if options[:use_both_names] | ||
"#{member.display_name} (#{member.fullname})" | ||
else | ||
member.display_name | ||
end |
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 don't know if this particular nesting is the best option -- IMO use_both_names
should take precedence when true and not be conditional on use_display_name
.
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.
Changed the function so it doesn't take options so this should not be an issue anymore
@@ -98,11 +98,16 @@ | |||
<dd><%= f.text_field :interests, class: "wide-input" %></dd> | |||
</dl> | |||
|
|||
<dl> | |||
<dt><%= f.label :prefers_full_name, "Prefer full name:" %></dt> | |||
<dd><%= f.check_box :prefers_full_name %></dd> |
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.
likewise I think it is a little more straightforward to say something like :show_real_names
, as we aren't really showing "full names" in the traditional sense and "prefers" is more ambiguous as to what the result is
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 agree somewhat with this. Technically in the code what is happening is that the fullname variable is being displayed. I think it's better to use only one name for this in the code.
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.
One small edit request: Make it "Always show full names of members:" The new distinction is the always.
Also, this PR does not currently change emails (e.g. |
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.
LGTM
"you?" if hover | ||
elsif !hover and current_member and current_member.prefers_full_name | ||
return er.assigned_to :both_names | ||
elsif !hover and current_member | ||
er.assigned_to use_display_name: true | ||
return er.assigned_to :display_name | ||
else | ||
er.assigned_to | ||
end |
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.
Can we use run_position_name_preference()
here too to be consistent? Maybe:
elsif !hover and current_member
return er.assigned_to run_position_name_preference(current_member)
That way if we add more options or change things in the future it lowers the number of places in the code to update.
@@ -98,11 +98,16 @@ | |||
<dd><%= f.text_field :interests, class: "wide-input" %></dd> | |||
</dl> | |||
|
|||
<dl> | |||
<dt><%= f.label :prefers_full_name, "Prefer full name:" %></dt> | |||
<dd><%= f.check_box :prefers_full_name %></dd> |
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.
One small edit request: Make it "Always show full names of members:" The new distinction is the always.
@@ -112,12 +112,16 @@ def to_s | |||
role + ": " + assigned_to | |||
end | |||
|
|||
def assigned_to(options = {}) | |||
def assigned_to(mode = :full_name) | |||
# Logger |
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.
Logger?
class AddProfileOptionViewFullName < ActiveRecord::Migration[6.1] | ||
def change | ||
# Change members table; Add column prefers_full_name | ||
add_column :members, :prefers_full_name, :boolean, default: false |
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.
Please also add null: false
.
Created migration to add :prefers_full_name to member. Added the option to edit it. Made it change the way names are displayed by event_role.