Display emoji statuses in more places!#5283
Conversation
chrisbobbe
force-pushed
the
pr-display-emoji-status
branch
2 times, most recently
from
7aecb28 to
ba9ea70
Compare
chrisbobbe
added
server release goal
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! Small comments below.
| // Set `color` just to remove some transparency that'd apparently get | ||
| // inherited, at least on Android, making emojis look noticeably | ||
| // faded. Remember, this component is basically an extension of RN's | ||
| // `Text` component, which has some style inheritance: | ||
| // https://reactnative.dev/docs/text#limited-style-inheritance | ||
| // See a screenshot of the faded appearance at | ||
| // https://github.com/zulip/zulip-mobile/pull/5277#issuecomment-1062504604 | ||
| // and the doc for this property at | ||
| // https://github.com/oblador/react-native-vector-icons#properties | ||
| color={color} |
There was a problem hiding this comment.
Hmm interesting. Glad that fixes it! Also gosh, I guess this has been affecting emoji elsewhere too -- in autocomplete, the picker for reactions, and the "who reacted" list.
I don't think the Text style inheritance can explain this, though -- at least, I don't think it can be inheriting the transparency from anything outside this UnicodeEmoji icon component itself. The docs you link to aren't 100% clear about this, but I believe what it means that the inheritance is "limited" is that it only comes from parents that are themselves Text components, and that that's what it's saying here:
React Native still has the concept of style inheritance, but limited to text subtrees.
In this case, even if this Emoji component doesn't count for this concept (I'm not sure if it would; possibly only native components, or something like that, might), its parent where we use it in e.g. AccountDetails is a View component. So I don't think any styles get inherited.
I don't think we need to debug this any further, though -- can just s/inherited/applied somehow/, and delete the sentence "Remember, … which has some style inheritance: …".
| * Users who have the "zero" status, `kUserStatusZero`, may be | ||
| * represented implicitly by having no record in this map. | ||
| */ | ||
| export const getUserStatus = (state: UserStatusesState, userId: UserId): UserStatus => |
There was a problem hiding this comment.
I'd much prefer to avoid giving this the same name as the other version; see: #5277 (comment)
I agree the other name getUserStatusFromModelState is pretty annoyingly long, but I'd rather take that than have the names collide.
One shortening would be …FromModel. I think "model" carries basically all the same information in this context as "model state" does.
| } | ||
| .status-emoji { | ||
| height: 1.4rem; | ||
| margin-left: 2px; |
There was a problem hiding this comment.
Given that it seems to be a bit of a mystery why this is cramped here but not on web, even though web also says margin-left: 2px and also seems to not have a space in the HTML, I think a fine strategy would be:
- Go ahead and make this 4px or 6px or whatever seems to make it a reasonable spacing, similar to web.
- Leave a 1- or 2-line comment saying it's different from web, and that's because more is mysteriously needed here in order to get the same effective spacing.
There must be an explanation for the discrepancy somewhere. But it'll be somewhere in the other CSS, and HTML, that's different between the two. Reconciling all that is a bigger job (and we probably never want it all to be the same).
Pretty much the worst-case consequence of making it 6px here instead of 2px is that in some future after we change something else to align with web, this spacing will be 4px too much -- so a pretty mild impact. And then if that does happen, a comment here explaining the discrepancy will help us spot the cause and fix it.
chrisbobbe
force-pushed
the
pr-display-emoji-status
branch
from
ba9ea70 to
b93d40d
Compare
|
Thanks for the review! Revision pushed. |
This covers several places where it's nice show the emoji status, for zulip#4925: - A 1:1 PM conversation item in the recent PMs list - The detail screen for a PM conversation (1:1 or group) - Users in the list of who reacted to a message - The share-with-who preview in share-to-Zulip - The searchable list of users to start a PM conversation Fixes-partly: zulip#4925
But with a warning in jsdoc, so callers don't misuse it (the concern that led us to un-export it in 1ba5138). The message list's BackgroundData has so far been a collection of stuff picked from PerAccountState, not an entire PerAccountState itself. It'll be easiest to not break that pattern when we start showing emoji statuses in the message list, coming soon. Discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user-statuses.20model/near/1340249
In particular, this fixes how an existing realm-emoji reaction renders. It also prepares us to add a test (coming soon) that uses that same realm emoji for a message sender's status emoji.
gnprice
force-pushed
the
pr-display-emoji-status
branch
from
b93d40d to
d671898
Compare
|
Thanks! Looks good; merging. |
|
Thanks for the review! |
2 participants
Following #5277 (review).
Fixes: #4925