-
-
Notifications
You must be signed in to change notification settings - Fork 827
Conversation
Signed-off-by: Tulir Asokan <[email protected]>
looks awesome, the inline svg is a bit out of place for the codebase |
Would be interesting to wire this up to the composer also :D |
Yeah, that was copied from emoji-mart since I didn't know where else to get those icons. I could probably split them into their own files
That probably requires some design input to figure out where to put it and what to do with the sticker picker |
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
@nadonomy There are some details the design wasn't clear on:
Also, currently I just approximated the sizes/margins/etc from the images, but if you have pixel precision designs, I can change the implementation to match it. |
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Hey @tulir— apologies I've been 99% afk for the last week or so, but adding this to my list to deep dive into. Do you have this deployed somewhere where I can easily play around with this? If not, just let me know and I'll set up my dev environment again on this machine. |
I should set up some gitlab pipelines to autodeploy riots, but for now I put it on https://riot.mau.dev/emoji-picker (I've also been using this on my main riot instance at https://matrix.maunium.net, but that has a bunch of other stuff and custom server selector disabled) |
Does this allow reacting with arbitrary text? It could be useful for polls (though I guess you could use an emoji-based index as well ^^" ) |
No, but I was planning to add that for myself. If @nadonomy makes a good UI for it, I can integrate it here. Maybe somehow hidden in the search box? |
In test instance widget looks very good, thanks! Will be good to have same widget in edit area for insert emoji to message text via same way! Does it hard to integrate current widget into message composer as separate "second smiley" button at first? |
Thanks for considering this, I'll try this version out! |
Functionally speaking (i haven't looked at the code), this is feeling really good - thank you for contributing it. @nadonomy unless you have particularly strong concerns, i'd recommend we just merge it asap and the iterate on it in place. for the record, Nad's official proposed design is: ...so it looks like we're pretty much there already. The only nastiness I can see from a quick play is that there's a ~1 pixel dead zone between the emoji options (at least on hidpi), so that if you scan the mouse over the available options you get a nasty flicker where it shows the quick selector whenever you jump from one to the next. |
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
Signed-off-by: Tulir Asokan <[email protected]>
this lgtm now, from a fairly high level pass through the code - i think we should merge it asap to see how it feels and then iterate on it further in place. i suggest someone (@jryans, @dbkr, @bwindels) does a retrospective line by line review, but would rather we get this in the hands of users asap. thanks again @tulir! |
@MurzNN That means your emojis are outdated, but not sure how that would happen. Riot should have the up to date (emoji 12 / 2019) twemoji font. I even specifically checked that those new wheelchair emojis worked. Chromium 69 sounds really outdated, maybe update? Anyway, this PR is done, issues should go in riot-web issues, not here. |
This fixes a regression introduced by the full emoji picker which inserted empty variation selectors in the thumbs up / down quick reactions. Since this makes them different characters, it would cause us to not aggregate thumbs from web vs. mobile together. Regressed by #3554 Fixes element-hq/element-web#11335
Fixes element-hq/element-web#9483
I decided to make a new emoji picker with inspiration from emoji-mart instead of trying to fix emoji-mart itself. Mostly because emoji-mart uses a different outdated emoji dataset than matrix-react-sdk and the code didn't look very nice to work with.
TODO
Future improvements
Some previews