Skip to content

Check for supported locale in Emoji picker#11079

Merged
ahukkanen merged 5 commits intodevelopfrom
fix/emojipicker-unsupported-locales
Jun 28, 2023
Merged

Check for supported locale in Emoji picker#11079
ahukkanen merged 5 commits intodevelopfrom
fix/emojipicker-unsupported-locales

Conversation

@andreslucena
Copy link
Copy Markdown
Member

@andreslucena andreslucena commented Jun 22, 2023

🎩 What? Why?

While working in Emojis, I found that we added i18n supported (yay!) but it fails if the locale isn't supported by our library providers (@picmo/popup-picker and emojibase).

The solution is to check if it's supported by them and if not fallback to English (as it used to work until now).

Sadly I couldn't find any API to get this list, so we're hardcoding the supported locales :/

📌 Related Issues

Testing

  1. Log in as an user
  2. Change your current locale to "catalan"
  3. Go to a comment textarea
  4. Click on the emoji button

📷 Screenshots

Before

Error in emoji picker in catalan

After

Fixed emoji picker in catalan

Mind that in this case emojipicker getting the correct locale but the problem is in emojibase that doesn't support catalan.

♥️ Thank you!

@ahukkanen ahukkanen mentioned this pull request Jun 22, 2023
Closed
ahukkanen
ahukkanen requested changes Jun 22, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Nice catch!

I added a recommendation how we could fetch the supported locales directly from the source (i.e. emojibase).

And just for clarity, this locale is used to describe each emoji picture as shown here:
Emoji translations

The UI translations (e.g. emoji categories and so) are shipped already with the Decidim locales.

Meanwhile looking at this I also noticed that the emoji data is loaded from a CDN. I created a new issue about that (see #11082), we can handle that separately.

@ahukkanen ahukkanen mentioned this pull request Jun 22, 2023
Closed
@andreslucena andreslucena added module: core type: fix PRs that implement a fix for a bug labels Jun 23, 2023
@andreslucena
Copy link
Copy Markdown
Member Author

andreslucena commented Jun 23, 2023

This is a bug that only happens in v0.28.0.dev, so we don't need to backport it. Maybe we should change the type: fix label to type: internal?

I'm thinking on a new procedure for making easier signaling these kind of exclusions for #11065. Also, focusing in the CHANGELOG, this isn't relevant to implementers, as it's a bug introduced (and solved) in v0.28.0.dev itself, so it makes sense to not add it there.

@andreslucena andreslucena mentioned this pull request Jun 27, 2023
Merged
@andreslucena andreslucena requested a review from ahukkanen June 28, 2023 07:01
@ahukkanen ahukkanen merged commit c584d4a into develop Jun 28, 2023
@ahukkanen ahukkanen deleted the fix/emojipicker-unsupported-locales branch June 28, 2023 07:50
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Jun 28, 2023

@andreslucena As a follow-up I think it might be good to try to get the locale data for Catalan also included in the emojibase packages.

I believe they are pulling the datasets for the translations from CLDR from this repository:
https://github.com/unicode-org/cldr

To me it seems that there is emoji data available for Catalan as well:
https://github.com/unicode-org/cldr/blob/main/common/annotations/ca.xml

I don't know their criteria for adding a new locale as it is not documented there (or I didn't find it). But maybe it could be suggested?
https://github.com/milesj/emojibase

@ahukkanen ahukkanen added the no-backport Pull Requests that should not be backported label Jun 28, 2023
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Jun 28, 2023

This is a bug that only happens in v0.28.0.dev, so we don't need to backport it. Maybe we should change the type: fix label to type: internal?

I'm thinking on a new procedure for making easier signaling these kind of exclusions for #11065. Also, focusing in the CHANGELOG, this isn't relevant to implementers, as it's a bug introduced (and solved) in v0.28.0.dev itself, so it makes sense to not add it there.

Regarding this I think a separate label is good for signaling this, as I've seen you've already added.

I have added the no-backport label to this and maintained the fix label with it. I think it is better to keep the original label as fix because it is a fix, although it does not concern the older versions.

@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Jun 28, 2023

I also found this issue where the maintainer said the following:

So emojibase only supports locales that have been requested by the community. Felt like overkill to support all of them from the beginning.

I asked a question there on what are the requirements and what is the process for suggesting a new locale:
milesj/emojibase#108 (comment)

@andreslucena
Copy link
Copy Markdown
Member Author

andreslucena commented Jun 29, 2023

I think it is better to keep the original label as fix because it is a fix, although it does not concern the older versions.

Yes, that was my reasoning too 👍🏽

I asked a question there on what are the requirements and what is the process for suggesting a new locale

👏🏽 👏🏽

entantoencuanto added a commit that referenced this pull request Jul 3, 2023
@entantoencuanto
Merge branch 'redesign/sync-develop-2' into 'feature/redesign-staging'
7138f79 
* redesign/sync-develop-2: (150 commits)
  Adapt tests to redesign
  Fix stylelint offenses
  Fix linter offenses
  Fix sanitizer
  Recover deleted translation
  Recover test fix
  Fix method definition and syntax
  Check for supported locale in Emoji picker (#11079)
  Fix configuration param and documentation links in CSP (#11098)
  Show all projects if none is selected when the voting has finished (#11090)
  Add Content Security Policy support (#10700)
  Replace `bootstrap-tagsinput` npm package with `tom-select` (#11076)
  Avoid password change to be requested when user registration mode is disabled (#11070)
  Lock sass-embedded to 1.62 (#11074)
  Add a button to send a newsletter to the admin (#10896)
  Fixing more tests
  Fixing more specs
  Fixing most of the failings specs
  Fixing most of the failings specs
  Bump doorkeeper from 5.5.4 to 5.6.6 (#11002)
  ...
andreslucena added a commit that referenced this pull request Jul 18, 2023
@andreslucena
Check for supported locale in Emoji picker (#11079)
e7e5222 
* Check for supported locale in Emoji picker

* Apply suggestion from code review

* Add spec

* Fix spellchecker offense

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

Reviewers

@ahukkanen ahukkanen ahukkanen approved these changes

Assignees

No one assigned

Labels

module: core no-backport Pull Requests that should not be backported type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andreslucena @ahukkanen