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

Make Nav.Tile id mandatory and update examples #3097

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Make Nav.Tile id mandatory and update examples #3097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a398fd
Make Nav.Tile id mandatory and update examples
phamduylong Jan 9, 2025
60966f6
Update tests
phamduylong Jan 9, 2025
2aa638d
Fix linter issues
phamduylong Jan 9, 2025
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/tall-pigs-perform.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@skeletonlabs/skeleton-svelte': patch
'@skeletonlabs/skeleton-react': patch
---

bugfix: Make `id` mandatory on Nav.Tile component
30 changes: 15 additions & 15 deletions packages/skeleton-react/src/lib/components/Navigation/Navigation.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ describe('Navigation', () => {
it('Renders the component', () => {
const { getByTestId } = render(
<Navigation.Rail>
<Navigation.Tile></Navigation.Tile>
<Navigation.Tile id="0"></Navigation.Tile>
</Navigation.Rail>
);
expect(getByTestId(testId)).toBeInTheDocument();
Expand All @@ -137,7 +137,7 @@ describe('Navigation', () => {
it('Renders the children', () => {
const { getByTestId } = render(
<Navigation.Rail>
<Navigation.Tile>
<Navigation.Tile id="0">
<div data-testid="child"></div>
</Navigation.Tile>
</Navigation.Rail>
Expand All @@ -148,7 +148,7 @@ describe('Navigation', () => {
for (const prop of ['base', 'width', 'aspect', 'background', 'hover', 'gap', 'rounded']) {
it(`Correctly applies the \`${prop}\` prop`, () => {
const value = 'foo';
const { getByTestId } = render(<Navigation.Tile {...{ [prop]: value }} />);
const { getByTestId } = render(<Navigation.Tile id="0" {...{ [prop]: value }} />);
expect(getByTestId(testId)).toHaveClass(value);
});
}
Expand All @@ -158,14 +158,14 @@ describe('Navigation', () => {
const expandedPadding = 'expandedPadding';
const { getByTestId, rerender } = render(
<Navigation.Rail expanded={false}>
<Navigation.Tile padding={padding} expandedPadding={expandedPadding}></Navigation.Tile>);
<Navigation.Tile id="0" padding={padding} expandedPadding={expandedPadding}></Navigation.Tile>);
</Navigation.Rail>
);
expect(getByTestId(testId)).toHaveClass(padding);
expect(getByTestId(testId)).not.toHaveClass(expandedPadding);
rerender(
<Navigation.Rail expanded={true}>
<Navigation.Tile padding={padding} expandedPadding={expandedPadding}></Navigation.Tile>);
<Navigation.Tile id="0" padding={padding} expandedPadding={expandedPadding}></Navigation.Tile>);
</Navigation.Rail>
);
expect(getByTestId(testId)).not.toHaveClass(padding);
Expand All @@ -177,14 +177,14 @@ describe('Navigation', () => {
const expandedGap = 'expandedGap';
const { getByTestId, rerender } = render(
<Navigation.Rail expanded={false}>
<Navigation.Tile gap={gap} expandedGap={expandedGap}></Navigation.Tile>);
<Navigation.Tile id="0" gap={gap} expandedGap={expandedGap}></Navigation.Tile>);
</Navigation.Rail>
);
expect(getByTestId(testId)).toHaveClass(gap);
expect(getByTestId(testId)).not.toHaveClass(expandedGap);
rerender(
<Navigation.Rail expanded={true}>
<Navigation.Tile gap={gap} expandedGap={expandedGap}></Navigation.Tile>);
<Navigation.Tile id="0" gap={gap} expandedGap={expandedGap}></Navigation.Tile>);
</Navigation.Rail>
);
expect(getByTestId(testId)).not.toHaveClass(gap);
Expand All @@ -196,14 +196,14 @@ describe('Navigation', () => {
const expandedClasses = 'expandedClasses';
const { getByTestId, rerender } = render(
<Navigation.Rail expanded={false}>
<Navigation.Tile classes={classes} expandedClasses={expandedClasses}></Navigation.Tile>);
<Navigation.Tile id="0" classes={classes} expandedClasses={expandedClasses}></Navigation.Tile>);
</Navigation.Rail>
);
expect(getByTestId(testId)).toHaveClass(classes);
expect(getByTestId(testId)).not.toHaveClass(expandedClasses);
rerender(
<Navigation.Rail expanded={true}>
<Navigation.Tile classes={classes} expandedClasses={expandedClasses}></Navigation.Tile>);
<Navigation.Tile id="0" classes={classes} expandedClasses={expandedClasses}></Navigation.Tile>);
</Navigation.Rail>
);
expect(getByTestId(testId)).not.toHaveClass(classes);
Expand All @@ -214,13 +214,13 @@ describe('Navigation', () => {
const active = 'active';
const { getByTestId, rerender } = render(
<Navigation.Rail>
<Navigation.Tile active={active} selected={false}></Navigation.Tile>
<Navigation.Tile id="0" active={active} selected={false}></Navigation.Tile>
</Navigation.Rail>
);
expect(getByTestId(testId)).not.toHaveClass('active');
rerender(
<Navigation.Rail>
<Navigation.Tile active={active} selected={true}></Navigation.Tile>
<Navigation.Tile id="0" active={active} selected={true}></Navigation.Tile>
</Navigation.Rail>
);
expect(getByTestId(testId)).toHaveClass('active');
Expand All @@ -232,7 +232,7 @@ describe('Navigation', () => {
it('Renders the component', () => {
const { getByTestId } = render(
<Navigation.Rail>
<Navigation.Tile label="label"></Navigation.Tile>
<Navigation.Tile id="0" label="label"></Navigation.Tile>
</Navigation.Rail>
);
expect(getByTestId(testId)).toBeInTheDocument();
Expand All @@ -244,7 +244,7 @@ describe('Navigation', () => {
const value = 'foo';
const { getByTestId } = render(
<Navigation.Rail>
<Navigation.Tile label="label" {...{ [prop]: value }}></Navigation.Tile>
<Navigation.Tile id="0" label="label" {...{ [prop]: value }}></Navigation.Tile>
</Navigation.Rail>
);
expect(getByTestId(testId)).toHaveClass(value);
Expand All @@ -258,7 +258,7 @@ describe('Navigation', () => {
it('Renders the component', () => {
const { getByTestId } = render(
<Navigation.Rail expanded={true}>
<Navigation.Tile labelExpanded="labelExpanded"></Navigation.Tile>
<Navigation.Tile id="0" labelExpanded="labelExpanded"></Navigation.Tile>
</Navigation.Rail>
);
expect(getByTestId(testId)).toBeInTheDocument();
Expand All @@ -270,7 +270,7 @@ describe('Navigation', () => {
const value = 'foo';
const { getByTestId } = render(
<Navigation.Rail expanded={true}>
<Navigation.Tile labelExpanded="labelExpanded" {...{ [prop]: value }}></Navigation.Tile>
<Navigation.Tile id="0" labelExpanded="labelExpanded" {...{ [prop]: value }}></Navigation.Tile>
</Navigation.Rail>
);
expect(getByTestId(testId)).toHaveClass(value);
Expand Down
2 changes: 1 addition & 1 deletion packages/skeleton-react/src/lib/components/Navigation/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export interface NavRailProps extends NavCommonProps {

export interface NavTileProps {
/** Provide a unique ID. */
id?: string;
id: string;
/** Provide an href link; turns Tiles into an anchor */
href?: string;
/** Set the href target attribute. */
Expand Down
8 changes: 4 additions & 4 deletions packages/skeleton-svelte/src/lib/components/Navigation/NavRail.test.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@
<!-- Rail -->
<Navigation.Rail {...rootProps}>
{#snippet header()}
<Navigation.Tile labelExpanded="HeaderTileExpanded" href="/" {...childProps}>HeaderTile</Navigation.Tile>
<Navigation.Tile id="0" labelExpanded="HeaderTileExpanded" href="/" {...childProps}>HeaderTile</Navigation.Tile>
{/snippet}
{#snippet tiles()}
<Navigation.Tile id="0" label="Files" href="/" {...childProps}>TileOne</Navigation.Tile>
<Navigation.Tile id="1" label="Images" href="/" {...childProps}>TileTwo</Navigation.Tile>
<Navigation.Tile id="1" label="Files" href="/" {...childProps}>TileOne</Navigation.Tile>
<Navigation.Tile id="2" label="Images" href="/" {...childProps}>TileTwo</Navigation.Tile>
{/snippet}
{#snippet footer()}
<Navigation.Tile labelExpanded="FooterTileExpanded" href="/" {...childProps}>FooterTile</Navigation.Tile>
<Navigation.Tile id="3" labelExpanded="FooterTileExpanded" href="/" {...childProps}>FooterTile</Navigation.Tile>
{/snippet}
</Navigation.Rail>
2 changes: 1 addition & 1 deletion packages/skeleton-svelte/src/lib/components/Navigation/NavTile.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
}
</script>

<!-- @component An individual Navgiation component tile. -->
<!-- @component An individual Navigation component tile. -->

<svelte:element
this={element}
Expand Down
2 changes: 1 addition & 1 deletion packages/skeleton-svelte/src/lib/components/Navigation/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export interface NavRailProps extends NavCommonProps {

export interface NavTileProps {
/** Provide a unique ID. */
id?: string;
id: string;
/** Provide an href link; turns Tiles into an anchor */
href?: string;
/** Set the href target attribute. */
Expand Down
2 changes: 1 addition & 1 deletion packages/skeleton-svelte/src/routes/components/navigation/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@
</Navigation.Tile>
{/snippet}
{#snippet footer()}
<Navigation.Tile labelExpanded="Settings" href="/settings" title="Settings"><IconSettings /></Navigation.Tile>
<Navigation.Tile id="5" labelExpanded="Settings" href="/settings" title="Settings"><IconSettings /></Navigation.Tile>
{/snippet}
</Navigation.Rail>
</div>
Expand Down
12 changes: 6 additions & 6 deletions sites/next.skeleton.dev/src/examples/components/navigation/Example.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,24 @@
<!-- Component -->
<Navigation.Rail>
{#snippet header()}
<Navigation.Tile href="/" title="Menu"><IconMenu /></Navigation.Tile>
<Navigation.Tile id="0" href="/" title="Menu"><IconMenu /></Navigation.Tile>
{/snippet}
{#snippet tiles()}
<Navigation.Tile id="0" label="Files" href={hrefExample}>
<Navigation.Tile id="1" label="Files" href={hrefExample}>
<IconFolder />
</Navigation.Tile>
<Navigation.Tile id="1" label="Images" href={hrefExample}>
<Navigation.Tile id="2" label="Images" href={hrefExample}>
<IconImage />
</Navigation.Tile>
<Navigation.Tile id="2" label="Music" href={hrefExample}>
<Navigation.Tile id="3" label="Music" href={hrefExample}>
<IconMusic />
</Navigation.Tile>
<Navigation.Tile id="3" label="Videos" href={hrefExample}>
<Navigation.Tile id="4" label="Videos" href={hrefExample}>
<IconVideo />
</Navigation.Tile>
{/snippet}
{#snippet footer()}
<Navigation.Tile labelExpanded="Settings" href="/settings" title="settings"><IconSettings /></Navigation.Tile>
<Navigation.Tile id="5" labelExpanded="Settings" href="/settings" title="settings"><IconSettings /></Navigation.Tile>
{/snippet}
</Navigation.Rail>
<!-- Content -->
Expand Down
12 changes: 6 additions & 6 deletions sites/next.skeleton.dev/src/examples/components/navigation/Example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ export const Page: React.FC = () => {
{/* Component */}
{/* prettier-ignore */}
<Navigation.Rail
header={<Navigation.Tile href="/" title="Menu"><IconMenu /></Navigation.Tile>}
footer={<Navigation.Tile href="/settings" title="settings"><IconSettings /></Navigation.Tile>}
header={<Navigation.Tile id="0" href="/" title="Menu"><IconMenu /></Navigation.Tile>}
footer={<Navigation.Tile id="1" href="/settings" title="settings"><IconSettings /></Navigation.Tile>}
>
<Navigation.Tile id="0" label="Files" href="#"><IconFolder /></Navigation.Tile>
<Navigation.Tile id="1" label="Images" href="#"><IconImage /></Navigation.Tile>
<Navigation.Tile id="2" label="Music" href="#"><IconMusic /></Navigation.Tile>
<Navigation.Tile id="3" label="Videos" href="#"><IconVideo /></Navigation.Tile>
<Navigation.Tile id="2" label="Files" href="#"><IconFolder /></Navigation.Tile>
<Navigation.Tile id="3" label="Images" href="#"><IconImage /></Navigation.Tile>
<Navigation.Tile id="4" label="Music" href="#"><IconMusic /></Navigation.Tile>
<Navigation.Tile id="5" label="Videos" href="#"><IconVideo /></Navigation.Tile>
</Navigation.Rail>
{/* Content */}
<div className="flex items-center justify-center">
Expand Down
14 changes: 7 additions & 7 deletions sites/next.skeleton.dev/src/examples/components/navigation/ExampleExpanded.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,27 @@
<!-- Component -->
<Navigation.Rail expanded>
{#snippet header()}
<Navigation.Tile href="/" labelExpanded="Menu"><IconMenu /></Navigation.Tile>
<Navigation.Tile id="0" href="/" labelExpanded="Menu"><IconMenu /></Navigation.Tile>
{/snippet}
{#snippet tiles()}
<Navigation.Tile id="0" labelExpanded="Browse Files" href={hrefExample}>
<Navigation.Tile id="1" labelExpanded="Browse Files" href={hrefExample}>
Copy link
Contributor

Choose a reason for hiding this comment

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

@phamduylong this is one of the only unique components we introduced that doesn't use Zag (intentionally). Now that I look at this, I wonder if rather than making ID mandatory, we utilize useId() to set a default automatically.

We didn't have access to useId() in Svelte when this component was built - that was introduced only when we integrated Zag. React has it built in as part of the library.

I think that'll provide a better experience - the ID will always be set, but users can still overwrite with a custom ID as they wish. Which would mean id would be optional again. Once less setting users have to deal with.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works for me. A bit less work for users, I wouldn't like to specify these if they aren't practically useful for my project either. I'll implement that.

<IconFolder />
</Navigation.Tile>
<Navigation.Tile id="1" labelExpanded="Browse Images" href={hrefExample}>
<Navigation.Tile id="2" labelExpanded="Browse Images" href={hrefExample}>
<IconImage />
</Navigation.Tile>
<Navigation.Tile id="2" labelExpanded="Browse Music" href={hrefExample}>
<Navigation.Tile id="3" labelExpanded="Browse Music" href={hrefExample}>
<IconMusic />
</Navigation.Tile>
<Navigation.Tile id="2" labelExpanded="Browse Videos" href={hrefExample}>
<Navigation.Tile id="4" labelExpanded="Browse Videos" href={hrefExample}>
<IconVideo />
</Navigation.Tile>
<Navigation.Tile id="2" labelExpanded="Browse Games" href={hrefExample}>
<Navigation.Tile id="5" labelExpanded="Browse Games" href={hrefExample}>
<IconGames />
</Navigation.Tile>
{/snippet}
{#snippet footer()}
<Navigation.Tile labelExpanded="Settings" href="/settings" title="Settings"><IconSettings /></Navigation.Tile>
<Navigation.Tile id="6" labelExpanded="Settings" href="/settings" title="Settings"><IconSettings /></Navigation.Tile>
{/snippet}
</Navigation.Rail>
<!-- Content -->
Expand Down
14 changes: 7 additions & 7 deletions sites/next.skeleton.dev/src/examples/components/navigation/ExampleExpanded.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ export const Page: React.FC = () => {
{/* prettier-ignore */}
<Navigation.Rail
expanded
header={<Navigation.Tile href="/" labelExpanded="Menu"><IconMenu /></Navigation.Tile>}
footer={<Navigation.Tile href="/settings" labelExpanded="Settings"><IconSettings /></Navigation.Tile>}
header={<Navigation.Tile id="0" href="/" labelExpanded="Menu"><IconMenu /></Navigation.Tile>}
footer={<Navigation.Tile id="1" href="/settings" labelExpanded="Settings"><IconSettings /></Navigation.Tile>}
>
<Navigation.Tile id="0" labelExpanded="Browse Files" href="#"><IconFolder /></Navigation.Tile>
<Navigation.Tile id="1" labelExpanded="Browse Images" href="#"><IconImage /></Navigation.Tile>
<Navigation.Tile id="2" labelExpanded="Browse Music" href="#"><IconMusic /></Navigation.Tile>
<Navigation.Tile id="3" labelExpanded="Browse Videos" href="#"><IconVideo /></Navigation.Tile>
<Navigation.Tile id="4" labelExpanded="Browse Games" href="#"><IconGames /></Navigation.Tile>
<Navigation.Tile id="2" labelExpanded="Browse Files" href="#"><IconFolder /></Navigation.Tile>
<Navigation.Tile id="3" labelExpanded="Browse Images" href="#"><IconImage /></Navigation.Tile>
<Navigation.Tile id="4" labelExpanded="Browse Music" href="#"><IconMusic /></Navigation.Tile>
<Navigation.Tile id="5" labelExpanded="Browse Videos" href="#"><IconVideo /></Navigation.Tile>
<Navigation.Tile id="6" labelExpanded="Browse Games" href="#"><IconGames /></Navigation.Tile>
</Navigation.Rail>
{/* Content */}
<div className="flex items-center justify-center">
Expand Down
4 changes: 2 additions & 2 deletions sites/next.skeleton.dev/src/examples/components/navigation/ExampleTiles.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@
<IconBox />
</Navigation.Tile>
<!-- Add selected to button tiles to enable the active state -->
<Navigation.Tile id="0" label="Button" selected>
<Navigation.Tile id="1" label="Button" selected>
<IconBox />
</Navigation.Tile>
<!-- When adding an href, they are converted to anchors -->
<Navigation.Tile id="0" label="Anchor" href={hrefExample}>
<Navigation.Tile id="2" label="Anchor" href={hrefExample}>
<IconBox />
</Navigation.Tile>
</div>
4 changes: 2 additions & 2 deletions sites/next.skeleton.dev/src/examples/components/navigation/ExampleTiles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ export const Page: React.FC = () => {
<IconBox />
</Navigation.Tile>
{/* Add selected to button tiles to enable the active state */}
<Navigation.Tile id="0" label="Button" selected>
<Navigation.Tile id="1" label="Button" selected>
<IconBox />
</Navigation.Tile>
{/* When adding an href, they are converted to anchors */}
<Navigation.Tile id="0" label="Anchor" href="#">
<Navigation.Tile id="2" label="Anchor" href="#">
<IconBox />
</Navigation.Tile>
</div>
Expand Down
Loading