Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Get user pill profile remote data and show unrecognised rooms as links #1246

Merged
merged 6 commits into from
Jul 25, 2017

Conversation

lukebarnard1
Copy link
Contributor

Instead of relying on the local avatar/displayname of a user, request the data from the server and update the pill if it shows up.

This required a slight refactor which means we're not doing everything in render now. Also I noticed unknown rooms weren't being rendered at all! So now you get something that looks like a normal link but with the room alias/ID in it.

Instead of relying on the local avatar/displayname of a user, request the data from the server and update the pill if it shows up.

This required a slight refactor which means we're not doing everything in `render` now. Also I noticed unknown rooms weren't being rendered _at all_! So now you get something that looks like a normal link but with the room alias/ID in it.
@lukebarnard1 lukebarnard1 changed the title Get user pill profile remote data Get user pill profile remote data and show unrecognised rooms as links Jul 24, 2017
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

otherwise much nicer (almost want to think about splitting them into separate components and a base component, but let's not worry about that for now)

const isUserPill = prefix === '@';
const isRoomPill = prefix === '#' || prefix === '!';
doProfileLookup: function(userId, member) {
MatrixClientPeg.get().getProfileInfo(userId).done((resp) => {
Copy link
Member

Choose a reason for hiding this comment

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

We should probably start using then in new code instead of done

member.events.member = {
getContent: () => {
return {avatar_url: resp.avatar_url};
},
Copy link
Member

Choose a reason for hiding this comment

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

Ew for synthesising Member objects :(

Copy link
Contributor Author

@lukebarnard1 lukebarnard1 Jul 25, 2017

Choose a reason for hiding this comment

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

Totally agree but sadly, this is the only way to update the member with a users's profile information. The best thing to do would probably be to fish out the important stuff from the membership event when it gets set instead of storing it with the member? Or do both, and set extra fields when setting the event but that also feels ming.

Said fields could then be clobbered, leaving the event out-of-date but that's strictly accurate.

FTR RoomMember only hangs on to its member event for the purposes of being able to return the correct avatar URL... But who knows what other layers do with member.events.member

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - problem is that we're using something that takes only a Member object, but sometimes we don't have one because we're just displaying a general user rather than a member of a room.

const isUserPill = prefix === '@';
const isRoomPill = prefix === '#' || prefix === '!';
doProfileLookup: function(userId, member) {
MatrixClientPeg.get().getProfileInfo(userId).done((resp) => {
Copy link
Member

Choose a reason for hiding this comment

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

Needs an unmounted guard

switch (this.state.pillType) {
case Pill.TYPE_USER_MENTION: {
// If this user is not a member of this room, default to the empty member
// TODO: This could be improved by doing an async profile lookup
Copy link
Member

Choose a reason for hiding this comment

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

outdated comment?

@lukebarnard1
Copy link
Contributor Author

almost want to think about splitting them into separate components and a base component, but let's not worry about that for now

I did think about this because Pill is increasingly covered in switches and could definitely just wrap either UserPill or RoomPill and magically the CSS classes will align.

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Jul 25, 2017
@lukebarnard1 lukebarnard1 merged commit b372e5d into develop Jul 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants