Skip to content
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

Change contact list on profile page #4360

Merged
merged 1 commit into from Aug 19, 2013

Conversation

Projects
None yet
5 participants
@svbergerem
Copy link
Member

svbergerem commented Aug 7, 2013

This will change the contact list on the profile page. If you look at your own profile a list of 8 contacts will be shown and under the list is a 'see all'-link to your own contacts-page. If you are on another users profile and the user shares a list of contacts with you, you will see the list the same way, with a link to the users contacts-page.

All in all the contacts are now part of the profile information.

diaspora_profile003

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 7, 2013

\o/

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 7, 2013

Nice work, @svbergerem . Can I just check that this only displays the contacts whom that person has made available to you by ticking 'Make contacts in this aspect visible to each other'? In other words,

It's one of the big ideas of Diaspora that the people each user are in contact with is not known to anyone else. The one exception to this (the only one I'm aware of) is when a person is able to see who else is in a particular aspect that they themselves are in.

If this feature displays only people in the same aspect(s) as me and which the person has chosen to make visible to me, that's fine. If it allows me to see all their contacts, that's a major privacy leak and we should discuss this in Loomio before doing any more, as it would be a major change of direction for Diaspora in terms of user privacy.

@svbergerem

This comment has been minimized.

Copy link
Member Author

svbergerem commented Aug 7, 2013

@goobertron As most of the code is copy&paste the behavior shouldn't change with this PR. There are three things which change:

  1. The style is slightly different as the contacts are now part of the profile_information-div
  2. We always link to the contacts-page, even if there are less then eight contacts
  3. On your own profile you see your own contacts and a link to the contacts page

I just wrote a cucumber test to make sure that you only see contacts when the aspect list is visible. The test is failing on my machine but so are similar tests too, which work for travis.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Aug 8, 2013

The test is failing here too.

@svbergerem

This comment has been minimized.

Copy link
Member Author

svbergerem commented Aug 8, 2013

Added changelog, squashed everything to one commit. The test now passes on my machine.

@svbergerem

This comment has been minimized.

Copy link
Member Author

svbergerem commented Aug 8, 2013

The refactoring of contact_count is much better done in #4369. When someone wants to pull this PR before #4369 I'll copy the refactored files from there.

Edit: I copied the changed files.

@goobertron

This comment has been minimized.

Copy link

goobertron commented Aug 8, 2013

Thanks @svbergerem for your reply.

@juliaguar

View changes

config/locales/diaspora/en.yml Outdated
@@ -590,6 +590,7 @@ en:
location: "location"
gender: "gender"
born: "birthday"
contacts: "contacts"

This comment has been minimized.

@juliaguar

juliaguar Aug 9, 2013

Contributor

This there a reason why you add a new translation here instead of using the already existing _contacts from line 45?

This comment has been minimized.

@svbergerem

svbergerem Aug 9, 2013

Author Member

_contacts starts with an uppercase 'C' but everything else in the profile sidebar is lower-case.

This comment has been minimized.

@juliaguar

juliaguar Aug 9, 2013

Contributor

true. still in other languages everything in the profile side bar starts with uppercase letters. I'm just saying that adding a new contacts entry would mean that it has to be translated into all 70 languages. But if you want to use downcase letters you could also take contacts.zero from line 292.

This comment has been minimized.

@jhass

jhass Aug 9, 2013

Member

No, don't access a plural key directly.

This comment has been minimized.

@Flaburgan

Flaburgan Aug 9, 2013

Member

I think everything should be uppercased

@jhass

View changes

features/contacts.feature Outdated
And I add the person to my "Unicorns" aspect

Scenario: see own contacts on profile
And I am on "robert@grimm.grimm"'s page

This comment has been minimized.

@jhass

jhass Aug 18, 2013

Member

Okay, here too, the scheme is as follows:

  • Given for setup / preconditions
  • When to start a logical block of actions that lead to an expectation and
  • Then for that expectation.

Consecutive elements of those should be replaced by And.

Therefore every Scenario has to start with a When or a Given, in any case a logical block shouldn't start with And.

This comment has been minimized.

@svbergerem

svbergerem Aug 18, 2013

Author Member

done

This comment has been minimized.

@jhass

jhass Aug 18, 2013

Member

Great, thanks! I'll wait for #4369 to let you deal with the merge conflict since you're more experienced :)

This comment has been minimized.

@svbergerem

svbergerem Aug 18, 2013

Author Member

ok :-P

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 19, 2013

Rebase time :)

Steffen van Bergerem
@svbergerem

This comment has been minimized.

Copy link
Member Author

svbergerem commented Aug 19, 2013

done :)

changed the behaviour of _profile_sidebar.html.haml a bit. Would be nice if someone looks at that one before merging.

jhass added a commit that referenced this pull request Aug 19, 2013

@jhass jhass merged commit 50e7142 into diaspora:develop Aug 19, 2013

1 check was pending

default The Travis CI build is in progress
Details
@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 19, 2013

Thank you!

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 19, 2013

Uh, that edit came in late :P

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Aug 19, 2013

So all you did was not displaying the <li>s if there's no content for them? I see no problem with that ;)

@svbergerem

This comment has been minimized.

Copy link
Member Author

svbergerem commented Aug 19, 2013

you meant the < li > s? Yep thats the only thing I changed. I just wanted to make sure that there is no need for them to be there (quite unlikely)

@svbergerem svbergerem deleted the svbergerem:change-contact-list-on-profile branch Aug 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.