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

Redesign contacts page #5153

Merged
merged 4 commits into from Aug 27, 2014

Conversation

Projects
None yet
5 participants
@svbergerem
Copy link
Member

svbergerem commented Aug 24, 2014

I am trying to match the contacts page with the notifications page and to supersede the aspect edit pane. In addition to our current design you can rename an aspect without the aspect edit pane.

My Contacts (top: old, bottom: new)
contacts_my-contacts

Friends (top: old, bottom: new)
contacts_friends

Community Spotlight (top: old, bottom: new)
contacts_community-spotlight

People search (top: old, bottom: new)
contacts_people-search

@jhass jhass added this to the next-major milestone Aug 24, 2014

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 24, 2014

<3

Is there any functionality left that's only available over the aspect edit pane? If not I'd vote for dropping it.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 24, 2014

So this port the contact page to backbone, awesome, this code was kind of dirty...

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 24, 2014

The aspect edit pane provides a searchable (ugly) list of your contacts to add those to a specific aspect with one click.

We could also make the contacts list searchable but then we would have to drop the infinite scroll and load all contacts at once. (or otherwise this would become much more complicated)

AFAIK that is all what's left and I am not sure if we even need that anymore.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 24, 2014

@Flaburgan Don't worry, there is still some dirty code for the contacts page left. ;-) I'll see if I can include that later.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 24, 2014

Yeah, my workflow is to just search by name via the main search. But then I guess we should remove it in a separate PR.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 24, 2014

@svbergerem the problem you are talking about is presented in #3838 (see my reflexion there and especially @jhass proposition in #3838 (comment))

@svbergerem svbergerem force-pushed the svbergerem:redesign-contacts-page branch from 2bb0041 to 3ef523f Aug 25, 2014

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 25, 2014

The search input and the js parts are still missing.
bildschirmfoto von 2014-08-25 03 15 12
bildschirmfoto von 2014-08-25 03 15 33

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 25, 2014

What about the solution proposed by Jonne? We drop the facebox and display all contacts directly on the aspect page but we sort them to have contacts in the aspect first and we add a input type="text" to filter them easily?

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 25, 2014

Now that's what I call a port! Great work, Steffen.

@svbergerem svbergerem force-pushed the svbergerem:redesign-contacts-page branch from 3ef523f to 189fd39 Aug 25, 2014

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 25, 2014

@Flaburgan

Adding/Removing contacts is already working. I will move the adding part to backbone, add the search input field and think about a nice way to highlight the contacts that are in this aspect.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 25, 2014

Adding/Removing contacts is already working. I will move the adding part to backbone, add the search input field and think about a nice way to highlight the contacts that are in this aspect.

Awesome, dropping the facebox is nice imo.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 25, 2014

a nice way to highlight the contacts that are in this aspect.

Well maybe we could keep the facebox way here:
capture du 2014-08-25 12 47 51

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 25, 2014

@Flaburgan I don't like those buttons which only contain one icon. This is how I separated the contacts in a first iteration:

I added the search input, moved that js code to backbone and also moved the aspect membership save/destroy to backbone. Destroying works great, saving doesn't call the success function but does the right stuff on the server. The error functions also need some improvements. ;-)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 25, 2014

Well imo we should put some green one way or another on the people who are in the aspect. The grey background is not enough, it does even confuse because it means "not read" in the other parts of the application (notifications, conversations...).

end
end
@contacts_size = @contacts.length
end

This comment has been minimized.

@jhass

jhass Aug 25, 2014

Member

You can pull out the sort_by here:

contacts = case
....
@contacts = contacts.to_a.sort_by {|c| c.person.name }

This comment has been minimized.

@svbergerem

svbergerem Aug 25, 2014

Member

Already thought of that. AFAIK that would mix contacts_in_aspect and contacts_not_in_aspect so even on aspect pages the contacts would be only sorted by name.

This comment has been minimized.

@jhass

jhass Aug 25, 2014

Member

Okay, right. I toyed around a bit, no idea if that's anywhere near working:

def set_up_contacts
  type = params[:set].presence
  type ||= "by_aspect" if params[:a_id].present?
  type ||= "receiving"

  @contacts = contacts_by_type(type)
  @contacts_size = @contacts.length
end

def contacts_by_type(type)
  contacts = case type
    when "all"
      [current_user.contacts]
    when "only_sharing"
      [current_user.contacts.only_sharing]
    when "receiving"
      [current_user.contacts.receiving]
    when "by_aspect"
      @aspect = current_user.aspects.find(params[:a_id])
      @contacts_in_aspect = @aspect.contacts
      @contacts_not_in_aspect = current_user.contacts.where.not(contacts: {id: @contacts_in_aspect.pluck(:id) }))
      [@contacts_in_aspect, @contacts_not_in_aspect].map {|relation|
        relation.includes(:aspect_memberships)
      }
    else
      raise ArgumentError, "unknown type #{type}"
    end

  contacts.map {|relation|
    relation.includes(:person => :profile).to_a.tap {|contacts|
      contacts.sort_by! {|contact| contact.person.name }
    }
  }.inject(:+)
end

I don't like relying on the side effects of all those instance variables being set here, but currently don't have an obvious way around that.

This comment has been minimized.

@svbergerem

svbergerem Aug 25, 2014

Member

Seems to work :-P

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 25, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 25, 2014

That's better! I don't know what others think about it.

Maybe you should rise the width of the search field a little bit btw.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 25, 2014

@Flaburgan Good point. I'll let the search input field expand on focus to 250px. Would that be ok?

@svbergerem svbergerem force-pushed the svbergerem:redesign-contacts-page branch 2 times, most recently from f14e013 to 8528179 Aug 25, 2014

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 25, 2014

That green bar is a good solution, especially if the contacts in the aspect will be grouped together at the top of the list.

Another possible solution would be to make the 'remove from aspect' icon on the right more obviously differentiated from the 'add to aspect' icon - it could even be in green instead of having a green bar on the left. I'm not sure this would look that good, and I appreciate the subtlety of the current grey icons, but you could play with ideas for using those existing icons to differentiate in-aspect and out-of-aspect contacts, rather than adding a new element on the left.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 25, 2014

What about a check icon in a green circle which will becomes the cross icon allowing to remove when you hover it?

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 25, 2014

Here are 3 new versions. The green checkmark will turn into a black circled cross on hover. I think the first one is not obvious enough so I'd prefer the second or third version.


@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 25, 2014

I like the second version, maybe make the check mark slightly bigger or wider.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 25, 2014

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 25, 2014

Yep, like that :)

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 25, 2014

What about the check icon being with and in a green circle like the cross?

});
expect($('[id^="flash"]')).toBeErrorFlashMessage(
Diaspora.I18n.t('contacts.error_add')
);

This comment has been minimized.

@svbergerem

svbergerem Aug 26, 2014

Member

This test fails:
Expected { 0: HTMLNode, length: 1, prevObject: { 0: HTMLNode, context: HTMLNode, length: 1 }, context: HTMLNode, selector: '[id^="flash"]' } to be error flash message 'Couldn't add to the aspect :('.

I copied the code from spec/javascripts/app/views/aspect_membership_view_spec.js. Any idea what went wrong?

This comment has been minimized.

@svbergerem

svbergerem Aug 26, 2014

Member

I think I just found the error. m(
Forgot the add the user's name in the translation.

@svbergerem svbergerem force-pushed the svbergerem:redesign-contacts-page branch from 9c4b8ab to 408e60b Aug 26, 2014

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 26, 2014

@goobertron pale green background:
bildschirmfoto von 2014-08-26 19 35 55

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 26, 2014

@Flaburgan buttons:

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 26, 2014

@svbergerem I think the color should be the opposite. But the previous solution with pale green background is also nice.

@svbergerem svbergerem force-pushed the svbergerem:redesign-contacts-page branch 2 times, most recently from 5ec6b8d to b7333ff Aug 26, 2014

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 26, 2014

A green button to remove a contact sounds confusing but I agree that the colors in the screenshot are also wrong. I just pushed the version with the green background to the server.

I also squashed the commits a bit and added a changelog entry. This PR should be ready for a review.

@svbergerem svbergerem changed the title [WIP] Redesign contacts page Redesign contacts page Aug 26, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 26, 2014

Well the button is green, you click on it it becomes gray, it sounds logical to me, at least more than the opposite. But let's use the icons ;)

Steffen van Bergerem

@svbergerem svbergerem force-pushed the svbergerem:redesign-contacts-page branch from b7333ff to ff84ee0 Aug 26, 2014

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 26, 2014

Yeh, it is a difficult one to puzzle out: the button (or whatever element) should be green to say 'I'm in the aspect', but at the same time it suggests 'Press this to make something positive happen'. However, we already use that configuration - button is green to show person is in contacts, click to take out of contacts and button goes grey - in the aspect selector button elsewhere - hovercards, profile pages and the main contacts page. So using that configuration here, on buttons or however, is consistent with elsewhere.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Aug 26, 2014

@goobertron You can use the aspect membership dropdown to either add a user to an aspect or remove him. The button shows "you are sharing with this user" and when you click on the button you are able to select the aspects. IMO that is different from "click this green button and you will remove your contact from this aspect". However, like Flaburgan said: let's use the icons. :-P

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 26, 2014

OK, sure, I didn't phrase that very well. What I meant is that, like the aspect membership dropdown, the colour suggests the current status (green = you are currently sharing; grey = you are not currently sharing), rather than suggesting the out come of interacting with that button.

But sure, I'm happy with icons too :)

Thanks for your excellent work.

@jhass jhass merged commit ff84ee0 into diaspora:develop Aug 27, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details

jhass added a commit that referenced this pull request Aug 27, 2014

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 27, 2014

Thanks a lot!

@svbergerem svbergerem deleted the svbergerem:redesign-contacts-page branch Aug 27, 2014

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 27, 2014

@goobertron hey, we just noticed that the field to filter indicates "User search". This can confuse the user: this field only filters in the contacts of the user, not in all users known by the pod. What text should we use? "Contact search", "Contact filter", just "Filter" ?

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 27, 2014

It depends what job it does. It looks like search-by-name rather than filtering (which is what the left-hand menu does). I'd suggest 'Search your contacts' to make it absolutely clear.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Sep 29, 2014

@svbergerem hi. I just wanted to know how "interactive" the page is currently (I mean, interaction without reloading the page). I have an aspect with more than 20 contacts in it, when I remove some, the count in the left panel stays the same, and the icon to start a private conversation doesn't appear, is that the normal behavior actually or is that a bug?

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Sep 29, 2014

Well in fact even after reloading the page the conversation button is not here, I guess this is #5235 (I have more than a thousand contacts).

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Sep 29, 2014

@Flaburgan This page isn't "interactive" at all.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Sep 29, 2014

Well when you remove or add someone to an aspect you have a direct feedback so part of the page is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment