FEATURE: user status emoji #17025
FEATURE: user status emoji #17025
Conversation
app/assets/javascripts/discourse/app/components/user-card-contents.js
Outdated
Show resolved
Hide resolved
|
|
||
| @computed("emoji") | ||
| get emojiHtml() { | ||
| const emoji = escapeExpression(`:${this.emoji}:`); |
There was a problem hiding this comment.
what if emoji becomes null?
There was a problem hiding this comment.
I understand there's the default, but this is a function call, what if something else makes it null without calling the default function?
There was a problem hiding this comment.
I'm not strongly against handling possible null value here, but I'm not sure that we should be that protective.
We do not support user statuses without emojis, an attempt to send a status without emoji to the server will lead to a server error and now emojiHtml() gets called only when emoji is already set:
Yes, something else can set emoji to null, but the answer to the question What will happen then? is Tests will fail. And after seeing that failure, we'll need to prevent that thing from setting emoji to null.
We can add some protection and start returning an empty string from emojiHtml(). The modal will look like this then:
But in that case, tests will still fail and show us that we have a problem, so probably it doesn't worth adding additional code.
What do you think?
There was a problem hiding this comment.
Hmm, with this actually even tests won't fail:
| this.set("emojiPickerIsActive", false); | ||
|
|
||
| scheduleOnce("afterRender", () => { | ||
| document.querySelector(".btn-emoji").focus(); |
There was a problem hiding this comment.
It's usually safer to do document.querySelector(".btn-emoji")?.focus();
There was a problem hiding this comment.
I've made it safer as you suggested, but now have some doubts. Like with the previous comment, not sure if we have to be so protective.
The component is designed in a way that it always have an emoji button. Let's say we set focus without ?:
document.querySelector(".btn-emoji").focus()If somebody removes btn-emoji from the component, tests will fail, they will see that and remove the code for setting focus.
But if we set focus with ? and somebody removes btn-emoji, tests will be green. Chances are that the code for setting focus will remain in the code base doing nothing.
In a way, by handling a possible null value in the code we're saying "the null value can appear here, if it happens that's fine". But that isn't the case here. If .btn-emoji was removed, this code should be removed as well. An explicit exception may be better than silent swallowing of the problem.
But again, not strongly against adding ? (I've already added it). Curious, what do you think?
|
|
||
| // Button with cleared default styles | ||
| // -------------------------------------------------- | ||
| .btn-unstyled { |
There was a problem hiding this comment.
I'm kinda mixed on this. What's your opinion @jordanvidrine ?
There was a problem hiding this comment.
I think using btn-flat would probably get the styling you want, without needing to make a new style that strips away things as it currently is.
If that doesnt work, this current class and styling is probably fine.
There was a problem hiding this comment.
Thank you, Jordan! btn-flat does the job, also with this style the button looks better in focused state. In Chrome, btn-unstyled:
btn-flat:
There was a problem hiding this comment.
Looking generally good. One thing I noticed as I pulled this locally is that we need some more events that clear the emoji modal. For example, from this screen:
I should be able to dismiss the emoji picker window if I
- click in the "What are you doing?" input
- click somewhere else in the modal content
- press Esc (currently this dismisses the whole modal; this is a little tricky, can be a todo for a future PR)
app/assets/stylesheets/common/components/user-status-picker.scss
Outdated
Show resolved
Hide resolved
|
|
||
| class DisallowNullEmojiOnUserStatus < ActiveRecord::Migration[7.0] | ||
| def up | ||
| execute "UPDATE user_statuses SET emoji = 'mega'" |
There was a problem hiding this comment.
📣 the megaphone feels like a very loud choice for a default emoji. Maybe 💬 (speech_balloon) is a better default? Or 🗨️ (left_speech_bubble).
| {{#if @emoji}} | ||
| {{html-safe this.emojiHtml}} | ||
| {{else}} | ||
| {{d-icon "discourse-emojis"}} |
There was a problem hiding this comment.
I think we should use the default icon right away here, instead of discourse-emojis, especially if using an emoji is not optional.
There was a problem hiding this comment.
I deliberately build in this behavior, put some time and efforts into it. May we merge it in and try it for a while first? We can always get rid of it in case we don't like it.
I've added handling of clicks outside of emoji picker. Also, after adding it, dismissing of the emoji picker by Esc key started working too. After dismissing by Esc focus behave a little weird, but I'd better prefer to fix it in separate PR since it can be nontrivial. |
Sure that's fine. Any thoughts on the default emoji icon? Since it's in the migration, it would be good to establish a good default before merging. |
I agree that 📣 is too loud, we also double-checked internally that we want to use 💬 instead. So I pushed the change. May I have an approval? At this moment this PR starts blocking my next work. |
This PR adds emoji to User Status: