FEATURE: Improve emoji diversity rendering #32909
Conversation
|
That was on purpose, but honestly no big feelings on if that should go that way or the other. My reasoning was: I store what has actually been used. My main complaint would probably be that it's more code for something that people will very rarely use (supposedly we don't change the diversity value very often). Anyways, if you feel like it should be like this, Im fine with it 👍 |
| this.emojiStore.diversity !== DEFAULT_DIVERSITY && | ||
| !/:t[1-6]$/.test(emoji) && | ||
| isSkinTonableEmoji(emoji) |
There was a problem hiding this comment.
I noticed that this logic is being duplicated in two spots in the codebase. Might be good for us to centralize it into a function somewhere.
There was a problem hiding this comment.
Good point, I moved the regex test into isSkinTonableEmoji(), where it more accurately fits the intended logic of the function.
| // Check if the emoji term already has a tone modifier | ||
| if (/:t[1-6]:?$/.test(term)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I don't know if I agree with this...
Technically :woman_artist:t1: can be changed to :woman_artist:t2: so it's a little bit surprising.
There was a problem hiding this comment.
Looking at the couple of places where isSkinTonableEmoji() was called in the codebase (prior to this PR), it appears that this is the expected behaviour: other usages will append a tone modifier if isSkinTonableEmoji() returns true.
There was a problem hiding this comment.
Ok fair, I guess I was not so surprised by this in the past :D
✨ What's This?
This change polishes the emoji diversity rendering behaviour in the emoji pickers. When a user changes their diversity setting, that setting will now be applied to favourites, chat default reactions, and in the emoji picker section selectors.
If an emoji was previously stored with a skin tone in favourites or default reactions, the stored skin tone won't be overridden.
📺 Screenshots