Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enh(NcEmojiPicker): Always show skin tone selector + save selection #5103

Merged
merged 4 commits into from Jan 23, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 22, 2024

☑️ Resolves

Always allow to select the skin tone used for emojis, this can be done by picking the skin tone on the left of the search bar.
The selected skin tone is saved on the local storage of the browser, so it is preserved during sessions.

Previously it was only possible to select a skin tone if the preview was enabled, but this has two problems:

  1. Not always possible to select
  2. ❗ The preview is for some emojis too crowed (too many shortcuts) so that the skin tone selector was not visible

Some minor other changes on components to implement this:

  • Allow to limit NcColorPicker to the given palette
  • Export Color class so we can reuse it
  • Fix Color to provide color attribute required by the color picker (otherwise one would have to provide it manually)

🖼️ Screenshots

vokoscreenNG-2024-01-22_01-32-48.mp4

🚧 Tasks

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

…n colors

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews feature: emoji picker Related to the emoji picker component labels Jan 22, 2024
@susnux susnux force-pushed the enh/emoji-picker-add-always-skintones branch 3 times, most recently from 6cce23e to 56c6945 Compare January 22, 2024 02:53
Copy link
Contributor

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good. Did not try it out though.

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! 2 questions:

  • Since there are 5 modifiers + default yellow, could the color dropdown be a 32 grid instead of 42 with only 2 in the bottom row? That would look cleaner. :)
  • Once this color is set, will it also be used in cases where you use : for emojis like in Text, Talk, Collectives, Mail, etc?

Thanks!

@susnux susnux added this to the 8.5.0 milestone Jan 23, 2024
Also save the selected skin tone for future uses and communicate it with all other NcEmojiPicker instances

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the enh/emoji-picker-add-always-skintones branch from 56c6945 to b2527dd Compare January 23, 2024 00:31
@susnux
Copy link
Contributor Author

susnux commented Jan 23, 2024

Since there are 5 modifiers + default yellow, could the color dropdown be a 32 grid instead of 42 with only 2 in the bottom row? That would look cleaner. :)

This comes from NcColorPicker and should probably be fixed there, would do so in a follow up.

@susnux susnux requested a review from ShGKme January 23, 2024 00:32
@jancborchardt
Copy link
Contributor

Since there are 5 modifiers + default yellow, could the color dropdown be a 32 grid instead of 42 with only 2 in the bottom row? That would look cleaner. :)

This comes from NcColorPicker and should probably be fixed there, would do so in a follow up.

@susnux sounds good! :)

Once this color is set, will it also be used in cases where you use : for emojis like in Text, Talk, Collectives, Mail, etc?

What about this one? Also cc @max-nextcloud for Text & Collectives, maybe you know?

@susnux
Copy link
Contributor Author

susnux commented Jan 23, 2024

Once this color is set, will it also be used in cases where you use : for emojis like in Text, Talk, Collectives, Mail, etc?

I need to test this but I think we need to adjust the emojiSeach function used in NcRichContenteditable. I will check an report.

…and use in `EmojiSearch`

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Jan 23, 2024

I need to test this but I think we need to adjust the emojiSeach function used in NcRichContenteditable. I will check an report.

No it did not work, but adjusting emojiSearch worked:

So @jancborchardt all points resolved and working 🎉

vokoscreenNG-2024-01-23_15-35-35.mp4

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, awesome job @susnux! :)

@susnux susnux merged commit 3ba708c into master Jan 23, 2024
16 checks passed
@susnux susnux deleted the enh/emoji-picker-add-always-skintones branch January 23, 2024 15:56
@Pytal Pytal mentioned this pull request Jan 23, 2024
@trailing-button-click="clearSearch(); slotProps.onSearch(search);"
@update:value="slotProps.onSearch(search)" />
<NcColorPicker palette-only
:palette="skinTonePalette"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a container here for inner NcPopover. Because of that, skin tones are rendered always on 'body'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: emoji picker Related to the emoji picker component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to change skin tone for emojis
5 participants