Skip to content

Conversation

@mejo-
Copy link
Member

@mejo- mejo- commented Jul 2, 2021

Summary

This is a first attempt to add the emoji picker to the text editor menubar. It works for me, but still has some rough edges.

Here's a screencast: recording

I'm curious to hear your thoughts @juliushaertl and @azul 😊

The way menu actions are organized in src/mixins/menubar.js at the moment unfortunately is a bit limited. Since the emoji picker isn't a simple <button> or <ActionButton> element and I prefer to reuse the <EmojiPicker> component from nextcloud-vue, I went a different route: I added the emoji action in src/mixins/menubar.js but special-treated it in the MenuBar component. I'm not particularly happy with this approach though. Maybe you have a better idea for a clean implementation?

@mejo- mejo- requested review from azul and juliusknorr July 2, 2021 17:29
@szaimen szaimen added this to the Nextcloud 23 milestone Jul 3, 2021
@mejo- mejo- requested a review from skjnldsv July 4, 2021 22:24
@mejo- mejo- force-pushed the enh/emoji_picker branch from 5bb41c6 to 7bedb98 Compare July 5, 2021 14:38
@szaimen

This comment has been minimized.

@juliusknorr
Copy link
Member

I'm not particularly happy with this approach though. Maybe you have a better idea for a clean implementation?

Yes, that one is indeed a bit tricky. The reason I went for the array instead of having the buttons directly in the component is that this allows to do some straight forward subset for rendering the popovermenu if the space exceeds the width. I might need to think about that a bit more.

@szaimen
Copy link
Contributor

szaimen commented Jul 13, 2021

And I think a rebase is probably needed...

@mejo- mejo- force-pushed the enh/emoji_picker branch 2 times, most recently from dc7dec8 to 25c17ee Compare July 15, 2021 09:14
@mejo-
Copy link
Member Author

mejo- commented Jul 15, 2021

Yes, that one is indeed a bit tricky. The reason I went for the array instead of having the buttons directly in the component is that this allows to do some straight forward subset for rendering the popovermenu if the space exceeds the width. I might need to think about that a bit more.

After thinking about it a second time, I'm more confident with my solution of special-treating the EmojiPicker in the for-loop.

Beware though that the way I implemented it results in the EmojiPicker being always displayed as the last non-hidden tool. That's on purpose, given that adding an emoji would require three clicks otherwise (open popovermenu, open emojipicker, select emoji).

Let me know what you think about it :)

@juliusknorr
Copy link
Member

I'm not particularly happy with this approach though. Maybe you have a better idea for a clean implementation?

Yes, that one is indeed a bit tricky. The reason I went for the array instead of having the buttons directly in the component is that this allows to do some straight forward subset for rendering the popovermenu if the space exceeds the width. I might need to think about that a bit more.

We could probably have a dedicated component with a slot that takes all the actions and then does conditional rendering based on the slot elements but not fully sure if that would be successful.

Something similar how it is done in the actions component https://github.com/nextcloud/nextcloud-vue/blob/master/src/components/Actions/Actions.vue#L649

@juliusknorr
Copy link
Member

Anyways I'd be fine with the current workaround for now. Doesn't seem too bad ;)

@mejo- mejo- force-pushed the enh/emoji_picker branch from 25c17ee to 6528d31 Compare July 15, 2021 09:59
@mejo- mejo- changed the title WIP: Add emoji picker to menubar (Fixes: #987) Add emoji picker to menubar (Fixes: #987) Jul 15, 2021
@mejo-
Copy link
Member Author

mejo- commented Jul 15, 2021

We could probably have a dedicated component with a slot that takes all the actions and then does conditional rendering based on the slot elements but not fully sure if that would be successful.

To be honest, I'm not sure whether it's worth the effort at the moment 😉. But you decide. If you'd like it to be that way, I'd give it a try.

Anyways I'd be fine with the current workaround for now. Doesn't seem too bad ;)

Yay 😊 Feel free to approve/merge. I just changed the PR from draft to ready to merge.

Signed-off-by: Jonas Meurer <jonas@freesources.org>
@mejo- mejo- force-pushed the enh/emoji_picker branch from 6528d31 to c108ccf Compare July 15, 2021 10:20
@mejo-
Copy link
Member Author

mejo- commented Jul 15, 2021

Rebased on latest master.

@juliusknorr juliusknorr merged commit d459c45 into master Jul 15, 2021
@juliusknorr juliusknorr deleted the enh/emoji_picker branch July 15, 2021 14:20
@mejo-
Copy link
Member Author

mejo- commented Jul 15, 2021

😍

@juliusknorr
Copy link
Member

Awesome contribution @mejo- 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make it easy to insert emojis

4 participants