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.

Sign up for GitHub

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

Fix type errors for Button's use of Icon data. #125

Merged
merged 1 commit into from
Nov 12, 2023
Merged

Fix type errors for Button's use of Icon data. #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/slimy-starfishes-report.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'svelte-ux': patch
---

Added new `IconInput` and `IconData` types to enable inclusive & seamless passing of icon arguments between components. Also provides a `asIconData` utility function for type-safe conversion.
Fixed type errors for Button & TextField's use of Icon data.
Copy link
Owner

Choose a reason for hiding this comment

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

<Icon> used to just accept <Icon path="...">

...then it was extended to allow <Icon svg="...">, ```

...then it was extended to have a simple <Icon data="..."> prop and infer which of the props was intended (path, svg, svgUrl), so I call that a feature to allow TextField to take in more options than just path :).

svgUrl is quite useful when your loading over the network, and not populating your bundle with all potential options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, I wasn't saying that it doesn't support different kinds of inputs. More that, we have instances of wrappers with inner wrappers with inner-inner Icons, etc. and the allowed data types for those wrapper components' distinct arguments do not always match, leading to inconsistency that has the potential to force type errors for no reason. I actually have to compliment this project for the diversity of data formats that Icon supports.

Copy link
Owner

Choose a reason for hiding this comment

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

:). It's been battled tested in a lot of use cases. One thing I struggle with is getting the time to show all the niceties in examples :). <Table> is under represented for how much it packs ;)

5 changes: 3 additions & 2 deletions packages/svelte-ux/src/lib/components/Button.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@
import type { TailwindColors } from '$lib/types';
import { getComponentTheme } from './theme';
import { getButtonGroup } from './ButtonGroup.svelte';
import { asIconData, type IconInput } from '$lib/utils/icons';

export let type: 'button' | 'submit' | 'reset' = 'button';
export let href: string | undefined = undefined;
export let target: string | undefined = undefined;
export let fullWidth: boolean = false;
export let icon: ComponentProps<Icon>['data'] | ComponentProps<Icon> | undefined = undefined;
export let icon: IconInput = undefined;
export let iconOnly = icon !== undefined && $$slots.default !== true;
export let actions: Actions<HTMLAnchorElement | HTMLButtonElement> | undefined = undefined;

Expand Down Expand Up @@ -261,7 +262,7 @@
<span in:slide={{ axis: 'x', duration: 200 }}>
{#if typeof icon === 'string' || 'icon' in icon}
<!-- font path/url/etc or font-awesome IconDefinition -->
<Icon data={icon} class={cls('pointer-events-none', theme.icon, classes.icon)} />
<Icon data={asIconData(icon)} class={cls('pointer-events-none', theme.icon, classes.icon)} />
{:else}
<Icon class={cls('pointer-events-none', theme.icon, classes.icon)} {...icon} />
{/if}
Expand Down
13 changes: 8 additions & 5 deletions packages/svelte-ux/src/lib/components/Icon.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
export let height = size;
export let viewBox = '0 0 24 24';
export let path: string | string[] = '';
export let data: IconDefinition | string | undefined = undefined;
export let data: IconDefinition | string | null | undefined = undefined;
export let svg: string | undefined = undefined;
export let svgUrl: string | undefined = undefined;

Expand All @@ -31,7 +31,7 @@
} = {};
const theme = getComponentTheme('Icon');

$: if (typeof data === 'object' && 'icon' in data) {
$: if (typeof data === 'object' && data && 'icon' in data) {
// Font Awesome
const [_width, _height, _ligatures, _unicode, _path] = data.icon;
viewBox = `0 0 ${_width} ${_height}`;
Expand Down Expand Up @@ -59,15 +59,18 @@
$: if (svgUrl) {
let promise;
if (cache.has(svgUrl)) {
cache.get(svgUrl).then((text) => (svg = text));
cache.get(svgUrl)?.then((text) => (svg = text));
} else {
promise = fetch(svgUrl)
.then((resp) => resp.text())
.catch((e) => {
.catch(() => {
// Failed request, remove promise so fetched again
cache.delete(svgUrl);
if (svgUrl && typeof(svgUrl) === "string") {
cache.delete(svgUrl);
}
// TODO: Consider showing error icon
// throw e;
return "";
});
cache.set(svgUrl, promise);
promise.then((text) => {
Expand Down
13 changes: 7 additions & 6 deletions packages/svelte-ux/src/lib/components/TextField.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,13 @@
import Button from './Button.svelte';
import Icon from './Icon.svelte';
import Input from './Input.svelte';
import { type IconInput, asIconData } from '$lib/utils/icons';

type InputValue = string | number;

const dispatch = createEventDispatcher<{
clear: null;
change: { value: typeof value; inputValue: InputValue; operator: string };
change: { value: typeof value; inputValue: InputValue; operator?: string };
}>();

export let name: string | undefined = undefined;
Expand All @@ -45,8 +46,8 @@
export let base = false;
export let rounded = false;
export let dense = false;
export let icon: string | null = null;
export let iconRight: string | null = null;
export let icon: IconInput = null;
export let iconRight: IconInput = null;
export let align: 'left' | 'center' | 'right' = 'left';
export let autofocus: boolean | Parameters<typeof autoFocus>[1] = false;
// TODO: Find way to conditionally set type based on `multiline` value
Expand Down Expand Up @@ -176,7 +177,7 @@
$: hasInputValue = inputValue != null && inputValue !== '';
$: hasInsetLabel = ['inset', 'float'].includes(labelPlacement) && label !== '';

$: hasPrepend = $$slots.prepend || icon != null;
$: hasPrepend = $$slots.prepend || !!icon;
$: hasAppend =
$$slots.append || iconRight != null || clearable || error || operators || type === 'password';
$: hasPrefix = $$slots.prefix || type === 'currency';
Expand Down Expand Up @@ -247,7 +248,7 @@
<slot name="prepend" />
{#if icon}
<span class="mr-3">
<Icon path={icon} class="text-black/50" />
<Icon data={asIconData(icon)} class="text-black/50" />
</span>
{/if}
</div>
Expand Down Expand Up @@ -417,7 +418,7 @@
{#if error}
<Icon path={mdiInformationOutline} class="text-red-500" />
{:else if iconRight}
<Icon path={iconRight} class="text-black/50" />
<Icon data={asIconData(iconRight)} class="text-black/50" />
{/if}
</div>
{/if}
Expand Down
17 changes: 17 additions & 0 deletions packages/svelte-ux/src/lib/utils/icons.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@

import type { ComponentProps } from "svelte";
import type { default as Icon } from "$lib/components/Icon.svelte";
import { isLiteralObject } from "./object";

export type IconInput = ComponentProps<Icon>['data'] | ComponentProps<Icon>;
export type IconData = ComponentProps<Icon>['data'];

export function asIconData(v: IconInput): IconData {
Copy link
Owner

Choose a reason for hiding this comment

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

What if we call this getIconData()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered that initially, and I'm good with that if you prefer it. Only reason I chose as was because get sounded like it was consistently extracting data, but just in potentially different/abstracted ways whereas as communicates that the object in question might already be the return type (IconData) and conveys that the function's purpose is ultimately type conversion rather than extraction. Up to you.

Copy link
Owner

Choose a reason for hiding this comment

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

We can leave it as is (pun intended) :)

return isIconComponentProps(v) ? v.data : v;
}

function isIconComponentProps(v: any): v is ComponentProps<Icon> {
// `iconName` is a required property of `IconDefinition`, the only other object that `IconInput` supports.
// If it is undefined, then only ComponentProps<Icon> is viable.
return isLiteralObject(v) && typeof(v['iconName']) === "undefined";
}