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

add avatar images to collaboration page #3047

Merged
merged 5 commits into from
Mar 14, 2018
Merged

Conversation

lgh2
Copy link
Contributor

@lgh2 lgh2 commented Feb 23, 2018

Adds avatar images next to username on the page which allows a package owner to modify collaborators (Warehouse/manage/project//collaboration/).

From what I have seen, the existing code already displays real names, if they exist.

@nlhkabu Would it be possible for you to review the styling?

Partially addresses issue #2875.

@di
Copy link
Member

di commented Feb 23, 2018

@lgh2 Looks pretty good!

screen shot 2018-02-23 at 2 47 28 pm

You're right that the user's full name is already there, so I would say that this PR definitely would fix #2875.

One thing I'm noticing is that each avatar links to the user's profile, but the username does not currently. It would probably be nice if both linked to the user's profile.

@lgh2
Copy link
Contributor Author

lgh2 commented Feb 23, 2018

@di Thank you! I will see about adding links to the usernames and full names.

<a href="{{ request.route_path('accounts.profile', username=role.user.username) }}" class="tooltipped tooltipped-e" aria-label="{{ role.user.username}}" data-original-label="{{ role.user.username}}">
<img src="{{ gravatar(role.user.email, size=50) }}" height="50" width="50" alt="{{ alt }}" title="{{ alt }}">
</a>
<a href="{{ request.route_path('accounts.profile', username=role.user.username) }}">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're missing the closing tag for this <a>.

Copy link
Contributor Author

@lgh2 lgh2 Feb 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add it, thank you!

@lgh2
Copy link
Contributor Author

lgh2 commented Feb 28, 2018

Hi @di! When you have a moment, could you take a look at this?

@di
Copy link
Member

di commented Feb 28, 2018

This LGTM, I think @nlhkabu will have some suggestions though (looks like the avatar needs some padding between it and the name)

@lgh2
Copy link
Contributor Author

lgh2 commented Feb 28, 2018

Yes, I would like to hear Nicole's thoughts. I think there is also an issue where a little line appears beside the avatar when the user hovers over them, and I'm not sure how to fix it.

@di
Copy link
Member

di commented Feb 28, 2018

@lgh2 That's because there is a "whitespace-only text node" in between the end of the <img> tag and the </a> tag:

screen shot 2018-02-28 at 1 04 48 pm

You'll need to put them all on one line with no whitespace in between.

@nlhkabu nlhkabu self-assigned this Mar 9, 2018
@lgh2 lgh2 force-pushed the avatars-on-collab-page branch 2 times, most recently from 641f487 to 188064e Compare March 12, 2018 18:00
@lgh2
Copy link
Contributor Author

lgh2 commented Mar 12, 2018

@nlhkabu I made the change Dustin suggested above and I think it fixed the issue.

We discussed putting the username and the real name on two separate lines, and I think you were going to paste some code related to that?

@nlhkabu
Copy link
Contributor

nlhkabu commented Mar 12, 2018

hi @lgh2 - yes, but I'm having some trouble getting everything to align nicely. I'll post something once I can get CSS to behave :/

@nlhkabu
Copy link
Contributor

nlhkabu commented Mar 13, 2018

hi @lgh2

So, the only solution I could come up with here was to wrap the avatar and names in a container div, and use flexbox to align them.

I also made the name and avatars have separate links, so we don't end up with ugly underlines in the middle of nowhere :)

<td class="table__user">
  <span class="table__user-details">
    <a href="{{ request.route_path('accounts.profile', username=role.user.username) }}" class="table__user-gravatar">
      {% set alt = "Avatar for {} from gravatar.com".format(role.user.username) %}
      <img src="{{ gravatar(role.user.email, size=50) }}" height="50" width="50" alt="{{ alt }}" title="{{ alt }}">
    </a>
    <a href="{{ request.route_path('accounts.profile', username=role.user.username) }}">
      <strong>{{ role.user.username }}</strong>
      {% if role.user.name %}
        <br>{{ role.user.name }}
      {% endif %}
    </a>
  </span>
</td>

This is the required CSS.
I would add it here: https://github.com/pypa/warehouse/blob/master/warehouse/static/sass/blocks/_table.scss#L382

The user-details container adds the flexbox styles, which will control the bahaviour of it's direct children (in the case the two <a>s.

Then I've simply applied a right margin to the gravatar to space it out a little.

.table__user-details {
  display: flex;
  align-items: center;
}

.table__user-gravatar {
  margin-right: 10px;
}

The result:

screenshot from 2018-03-13 06-57-40

This solution will need to be checked across different resolutions (desktop, tablet, mobile) to make sure it's working properly :)

@lgh2
Copy link
Contributor Author

lgh2 commented Mar 13, 2018

@nlhkabu thank you for your help! @di When you have a chance, would it be possible for you to take a look at this?

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants