fix: icon type auto-generation and normalization

[ethanalvizo]

Closes #601
Closes #784

Updated deephaven/ui README with instructions on how to run the script. Wasn't certain if it should run everytime the docker container is built?

[bmingles]

@bender we should consider changing something in @deephaven/icons so that we have a separate types file that only includes the icon names. This would help with some .ts scenarios as well so we don't have to exclude the stuff at the end of the file.

...
// end of current icons/dist/index.d.ts
import { IconDefinition, IconLookup, IconName, IconPrefix, IconPack } from '@fortawesome/fontawesome-common-types';
export { IconDefinition, IconLookup, IconName, IconPrefix, IconPack } from '@fortawesome/fontawesome-common-types';
export const prefix: IconPrefix;
export const vs: IconPack;
export const dh: IconPack;

As-is, this is potentially brittle to changes.

[mattrunyon]

You could modify the build.js script in @deephaven/icons to output a separate d.ts file with just the icon names as a type. If you look at the dtsIndex, you'll see how to get the name from the files array in build.js.

I also think our d.ts file is slightly wrong. It doesn't look like we export a vs namespace of all the icons.

Another option would be to use Node and do import * as icons from '@deephaven/icons';. Then iterate the icons obj and do a check like if ('prefix' in icon && 'iconName' in icon)

[ethanalvizo]

Can use the example below to validate snake case and camel case for icon names works.

For duplicate icons (ex. vsEye and dhEye) I default to the vsIcon if they enter the icon name with no prefix. So 'Eye' or 'eye' would render 'vsEye' but 'dhEye' or 'dh_eye' still renders 'dhEye'

from deephaven import ui


@ui.component
def ui_button_group():
    actionGroupWithItems = ui.action_group(
        ui.item(ui.icon("dhRefresh"), aria_label="Restart"),
        ui.item(ui.icon("vsRefresh"), aria_label="Record"),
        ui.item(ui.icon("vs_refresh"), aria_label="Play"),
        ui.item(ui.icon("dh_refresh"), aria_label="Pause"),
        ui.item(ui.icon("dhRefresh"), aria_label="Edit"),
        ui.item(ui.icon("Refresh"), aria_label="Configure"),
    )
    return [actionGroupWithItems]


my_action_buttons = ui_button_group()

[mofojed]

Just double check with @dsmmcken that we do want to include dh_refresh and other icons that conflict with the VS counterpart similarly.

[mofojed]

@dsmmcken I want to make sure we get this right...
So we have a dhPandas icon, and no vsPandas icon. Do we want the Literal IconTypes to only contain pandas and it points to dhPandas icon? And not have a dh_pandas? And then for something like refresh where we have vsRefresh and dhRefresh, we should just have refresh => vsRefresh and dh_refresh => dhRefresh?
Only concern is if we just have pandas => dhPandas and no dh_pandas, then if we update VS Code icons and then they add a vsPandas, then we'd have pandas => vsPandas, and dh_pandas => dhPandas. Meaning we'd be changing that icon. Would that be an issue?

[mofojed]

Or should we just always prefix our dh icons with dh_ and then we can avoid any conflicts altogether?

[dsmmcken]

I see arguments for both now that you lay it out that way. I guess I am good with always prefixing dh_ to handle potential future conflicts.

[ethanalvizo]

Also added this change now but leaving this unresolved so it can be seen during review.

[mattrunyon]

Why use Python and regex to parse the icons instead of just importing all the icons in JS and iterating the actual objects? The output file is written line by line and not using anything specific from Python to create the types.

It works, but IMO it makes a lot more sense to use Node and import * as Icons from '@deephaven/icons'; then iterate that to create your list. No regex and doesn't rely on specifics of the d.ts file.

[mattrunyon]

Also, why do we list multiple variants of some things in the icon mappings? Specifically we have things like Add and add as options. Add never worked previously. Neither did vs_add. So why are we adding those if we don't want people to use them and they've never worked? It should only be add and vsAdd since vsAdd previously worked.

Or only accept the allowed icon names and mark it as breaking that some icon names changed. I think that's fine since this is still pre-release/beta/whatever, but I know others on the team don't share my view.

Final note, this should not be a fix. Probably a feat

[mofojed]

@mattrunyon it's Python as in theory this should be part of the Python build process. Still manually generated right now so that's kind of a moot point. We also don't add icons very frequently.
True we probably don't need to be adding vs_add, but I don't mind adding them to the IconMapping if they do map to valid icons, only that they're not polluting the IconTypes literal.
I'm fine with fix or build, we're just being more specific with the types here. I guess you could argue a "feature" as now the user has the literal types for icons; that being said, you can't really autocomplete the literals from our IDE anyway so it's not really a feature that does a lot for them right now.

@ethanalvizo see @dsmmcken 's comment about dh icons, they should just be dh_, don't do a prefixless version of them in the types: #696 (comment)

[mofojed]

Minor cleanup looks good overall