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

Reformat the data-by-group.json to be an array rather than on object. #11

Merged
merged 3 commits into from
Oct 16, 2022
Merged

Reformat the data-by-group.json to be an array rather than on object. #11

merged 3 commits into from
Oct 16, 2022

Conversation

DaveBagler
Copy link
Contributor

This pull request changes the format of data-by-group.json. Rather than group names being property keys, the file is an array of group objects with name, slug and emojis properties.

Current Format

{
  "Smileys & Emotion": [
    {
      "emoji": "😀",
      "skin_tone_support": false,
      "name": "grinning face",
      "slug": "grinning_face",
      "unicode_version": "6.1",
      "emoji_version": "2.0"
    },
  ],
  ...
}

Format in this PR

[
  {
    "name": "Smileys & Emotion",
    "slug": "smileys_emotion",
    "emojis": [
      {
        "emoji": "😀",
        "skin_tone_support": false,
        "name": "grinning face",
        "slug": "grinning_face",
        "unicode_version": "1.0",
        "emoji_version": "1.0"
      },
      ...
    ]
  },
  ...
]

@muan
Copy link
Owner

muan commented Apr 29, 2022

Hi, thanks for the PR! Could you provide some context on why the change? I can see this may be handy in some cases but the DX improvement seems minimal to warrant a breaking change version bump for the package.

@DaveBagler
Copy link
Contributor Author

Hi @muan,
Thanks for responding so quickly, and sorry for the late reply.

The current JSON object requires array-style access and is harder to type in typescript. The array of group objects is much easier to type as a typescript interface.

Additionally, adding the slug gives a nice key to use when passing group names through a translation system.

@muan muan merged commit 41e9c31 into muan:main Oct 16, 2022
@muan
Copy link
Owner

muan commented Oct 16, 2022

Sorry for the delay in merging. Thanks for the work. I think it makes a lot of sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants