Group languages by locale so you can easily browse them

[gavinhenderson]

Fix for #31

Ideally it would show language rather than locale so its more understandable. However, there is no native way to get that information. The lang codes are strictly defined so you could pull a map of lang codes to languages fairly easily but it just seems like unnecessary data to send to the client.

I had to restructure the options to code to group grouping to work. Options now takes an object rather than an array. Each key is a group and each value in the object should be a list of values for the group.

There is a magic key of 'root' if you want to have a totally flat list.

[sjjhsjjh]

Instead of a magic value, could the property take either an array or an object? Then create a flat list if it's an array or a grouped list if it's an object.

[gavinhenderson]

@sjjhsjjh Cheers for the feedback.

That was my initial plan but the main drawback with that is that you still need the magic value when people use an object because you can mix top level options and option groups in the same select. So I figured if I had to leave the magic value in I might aswell make it the only way. However, im totally open to change it if you think that the array option aswell as the object with magic value is better.

Regarding reduce vs loop. My personal opinion is that I prefer loops, I find I can understand a for loop much quicker and easier than a reduce. Although I don't have a super strong opinion on it and know that its totally subjective so if you have a strong opinion I am happy to change it.

Here is both for a comparison.

Current:

let voiceGroups = {};

for (let i = 0; i < speech.voices.length; i++) {
    const currentVoice = speech.voices[i];

    if (voiceGroups[currentVoice.lang] === undefined) {
        voiceGroups[currentVoice.lang] = [currentVoice.name];
    } else {
        voiceGroups[currentVoice.lang].push(currentVoice.name);
    }
}

As Reduce:

const voiceGroups = speech.voices.reduce((accumulator, currentVoice) => {
    const langGroup = accumulator[currentVoice.lang] || [];

    return {
       ...accumulator,
       [currentVoice.lang]: [...langGroup, currentVoice.name]
    }
}, {})

[sjjhsjjh]

Hi Gavin,

I'm unsure that the magic key approach supports a mixed list though, because keys cannot be repeated in an object. Consider an option list like this:

• Actual option
• Another actual option.
• Group one.
  • Option in group 1
  • Another option in group 1
• Now back to more options

The endgame for declaring that might have to be an object like this:

{
    "Actual option": null,
    "Another actual option": null,
    "Group one.": [
        "Option in group 1",
        "Another option in group 1"
    ],
    "Now back to more options": null
}

Edit: or maybe like this:

  [
      "Actual option",
      "Another actual option",
      {"Group one.": ["Option in group 1", "Another option in group 1"]},
      "Now back to more options"
  ]

A mixed list of strings and objects, with each object having only one key. Actually, that'd be a little difficult to build from the language list.

I was seeing the reduce code like this in order to minimise new object and array allocation.

const voiceGroups = speech.voices.reduce((accumulator, voice) => {
    if (accumulator[voice.lang] === undefined) {
        accumulator[voice.lang] = [];
    }
    accumulator[voice.lang].push(voice.name);
    return accumulator;
}, {})

[gavinhenderson]

Very good point. I like your idea of it always being an array. I would make one slight amendment and store the groupTitle as a value rather than a key. like so:

const list = [
    {
       groupTitle: 'GroupName',
       values: ['value', 'another value']
    },
    {
       groupTitle: 'Second group name',
       values: ['value', 'another value']
    },
    'Root value',
     {
       groupTitle: 'Final group',
       values: ['value', 'another value']
    },
]

Regarding the reduce, I am not sure that the memory allocation makes much of a difference when we are talking about an array of this size and V8 often can optimise it to the same thing. Your reduce is definitely cleaner than my reduce but I still prefer using a for loop over a reduce, again though it comes down to personal preference for readability. If you feel strongly that reduce reads better I'm happy to change it.

[sjjhsjjh]

Hi Gavin, I agree with your proposed array structure. Maybe change groupTitle to label to be consistent with the optgroup attribute. I think we should just a functional programming style here to show that the project is up-to-date and to be consistent. Maybe .forEach is more expressive than .reduce in this case. Cheers, Jim

[netlify[bot]]

✔️ Deploy Preview for dasherv6demo ready!

🔨 Explore the source changes: 99494de6d6b3fa98711b9e7512450e8a9d412273

🔍 Inspect the deploy log: https://app.netlify.com/sites/dasherv6demo/deploys/60f2ec45ebdf330007fe33ab

😎 Browse the preview: https://deploy-preview-96--dasherv6demo.netlify.app

[gavinhenderson]

@sjjhsjjh I've changed it to that format and added a little comments, let me know your thoughts :)

[sjjhsjjh]

Hi @gavinhenderson , It turns out that the voice selection back end (my code) relied on the order in the selection drop-down being the same as the order in the Speech object. That was pretty poor coding by me so I fixed it to find the selected voice by name. Anyway, I think I have finished working on this so if you think it's fine you can merge. Cheers, Jim

[gavinhenderson]

@sjjhsjjh Thanks for sorting that! 👍

Your code all goods good to me, ill merge and close the issue