#378 Emoji suggestions #546
Conversation
Mavrin
force-pushed
the
emoji_suggestion
branch
from
4bfa65f
to
716fbe2
Compare
| import { StaticStore } from '@app/common/static_store'; | ||
| import { useEffect, useRef } from 'preact/hooks'; | ||
| import '@github/text-expander-element'; | ||
| import styles from './text-expander.module.pcss'; |
There was a problem hiding this comment.
I realized that we should use BEM because it gives the opportunity to customize interface just by including your own CSS file.
Otherwise, we should provide permanent class names for it.
I think we can move to css-modules but we need to change the whole CSS structure. And I think it can be convenient to change theming by setting root theme class besides using theme hook in every component. But it is a theme for a big issue. I think we can discuss it.
There was a problem hiding this comment.
Got it. I will make permanent class. About theme I think we should extract all color variable to css variables, but before we should drop support IE11 and provide some api to set that colors.
There was a problem hiding this comment.
Also, we can move to css-modules but with external permanent class names for customization. I think It will make the bundle smaller.
There was a problem hiding this comment.
if dropping IE11 is the decision I supposed to support - sure, go for it. My understanding - IE11 is not that popular anymore (2% ?) and we can drop it safely
There was a problem hiding this comment.
@umputun yep, it is less than 2% (https://caniuse.com/usage-table)
There was a problem hiding this comment.
Even Jira will remove support ie 11 https://blog.developer.atlassian.com/retiring-ie11-support-for-atlassian-cloud-server-and-data-center-products/. Also we can make fallback only on light theme.
There was a problem hiding this comment.
Now class will be look like remark42__text-expander-module__suggesterItem
| return Promise.resolve({ matched: false }); | ||
| } | ||
| const fragment = document.createDocumentFragment(); | ||
| render(<SuggestionList theme={theme} items={emojiList.slice(0, 5)} />, fragment); |
There was a problem hiding this comment.
I think the first 5 emojis are not relevant for daily use. I think we can choose "top 5 emojis" for first appear.
There was a problem hiding this comment.
It is good idea. It can be improved in the future. I don't know where I can this information, look like it depend on user.
There was a problem hiding this comment.
I found the rating, but I think we need to pick the most neutral of it. Like laughing, smiling, clapping, thumbs up or something like suggesting github/facebook for comment reaction.
https://home.unicode.org/emoji/emoji-frequency/
There was a problem hiding this comment.
Or it can be configurated :)
There was a problem hiding this comment.
just an idea: can we save the latest selections and display as defaults? As an initial, predefined default we can pick some from the list above.
| @@ -15,6 +15,7 @@ import { SubscribeByRSS } from './__subscribe-by-rss'; | |||
|
|
|||
| import MarkdownToolbar from './markdown-toolbar'; | |||
| import TextareaAutosize from './textarea-autosize'; | |||
| import { TextExpander } from './text-expander'; | |||
There was a problem hiding this comment.
Maybe we can make a chunk and load it only when emoji is enabled?
There was a problem hiding this comment.
expander component about 3k (https://bundlephobia.com/result?p=@github/text-expander-element@1.0.1). node-emojis will load only on first search. Of course it can be improved. But in current state will be better to move comment form to separate chunk. But it out of the scope this pull request.
Mavrin
force-pushed
the
emoji_suggestion
branch
from
ecb6c4c
to
3b38889
Compare
|
visually it's working nice, thx. I'm going to merge it as is and in case if you going to modify the default suggesting pls make a separate PR for this. |
No description provided.