Refactor modify aspect facebox #4567
Conversation
|
Nothing much to work on, waiting for a review. |
|
New commit to fix #4568 |
|
Thanks, Fla. That's a useful addition. Can't usefully review your code, I'm afraid, but adding the text 'Visibility' to the button will be very helpful, I think. |
|
👍 looks good |
| @@ -40,4 +40,17 @@ def aspect_membership_button(aspect, contact, person) | |||
| remove_from_aspect_button(membership.id, aspect.id, person.id) | |||
| end | |||
| end | |||
|
|
|||
| def aspect_visibility_link(aspect); | |||
There was a problem hiding this comment.
the ";" at the end of each line are not needed :)
|
@Flaburgan you should update one of the existing tests in order to cover this, I'll be back in a couple of hours to finish the review :) |
|
Well, the visibility is tested here: https://github.com/diaspora/diaspora/blob/develop/features/desktop/contacts.feature#L28 and I have nothing to change to adapt the test (the link is the same, I just changed its style). But by the way, these tests seem deprecated now that the default behavior is that contacts are not visible. So there is work to do, but it's not directly linked to this PR. |
| title = t('.aspect_list_is_not_visible') | ||
| end | ||
| padlock = content_tag(:div, nil, id: "contact_visibility_padlock", class: icon) | ||
| link_to "#{t('.visibility')} #{padlock}".html_safe, aspect_toggle_contact_visibility_path(@aspect), |
There was a problem hiding this comment.
I'm not really happy with this, but can you use #raw instead of #html_safe here?
|
Nice work so far. How about changing the linktext too? Instead of 'Visibility': 'Visible' <-> 'Invisible' |
|
@svbergerem in my next pull request (#4570), I changed it to "Set visibility" because the other buttons have action verbs too ("Manage, start a conversation, delete). |
|
alright, sounds good |
Refactor modify aspect facebox
|
all right, thanks @Flaburgan!! |
|
In production on diaspora-fr.org, looks great at the moment. |
Now that contacts are not visible to each other by default (see #4343), the user has to be able to change visibility easily. This can be done in the "edit aspect facebox", available in my_aspects view when hovering an aspect. I added it to the contact page too with #4397, but I still thought that the padlock to change visibility was not enough clear.
This PR goes from
to 
.
I also improved the text displayed when the user hover the button.
This PR also includes minor design arrangements and refactor of all the css from application.sass to facebox.scss, improving #3796