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

Jump to bottom

NEXT: Svelte Tabs Component #2541

Merged
merged 22 commits into from
Mar 29, 2024
Merged

NEXT: Svelte Tabs Component #2541

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
afeb545
init svelte tabGroup
Mar 9, 2024
14c5355
Merge branch 'next' of https://github.com/Mahmoud-zino/skeleton into …
Mar 13, 2024
37ac3b0
re-init tabs parent component
Mar 13, 2024
78dbb34
Merge branch 'next' of https://github.com/Mahmoud-zino/skeleton into …
Mar 15, 2024
2fe18b7
Merge branch 'next' of https://github.com/Mahmoud-zino/skeleton into …
Mar 15, 2024
7819679
init tabs control
Mar 15, 2024
9e0ae6c
init tabsPanel
Mar 16, 2024
d6338cc
applied ARIA guidelines
Mar 16, 2024
a4a8ea3
implemented events
Mar 16, 2024
6964420
using better html symantics
Mar 16, 2024
6842522
implement RTL changes
Mar 16, 2024
93a2153
renamed rxSelected to selected
Mar 18, 2024
c0dacc5
removed context props
Mar 25, 2024
6ef0dbb
Merge branch 'next' of https://github.com/Mahmoud-zino/skeleton into …
Mar 27, 2024
69982d3
mini style changes
Mar 27, 2024
18c5c37
moved a11y props
Mar 27, 2024
c3ad466
corrected a11y prop name
Mar 27, 2024
d47e9f7
update data-testId
Mar 27, 2024
532c7e8
applied some code suggestions
Mar 27, 2024
46707b6
Audit and changes
endigo9740 Mar 27, 2024
b78f01d
Second round audit changes
endigo9740 Mar 28, 2024
82d99eb
Merge branch 'next' into feat/tabs-next
endigo9740 Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 11 additions & 13 deletions packages/skeleton-svelte/src/app.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
<!doctype html>
<html lang="en" class="dark">
<head>
<meta charset="utf-8" />
<title>skeleton-svelte</title>
<link rel="icon" href="%sveltekit.assets%/favicon.png" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
%sveltekit.head%
</head>

<head>
<meta charset="utf-8" />
<title>skeleton-svelte</title>
<link rel="icon" href="%sveltekit.assets%/favicon.png" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
%sveltekit.head%
</head>

<body data-sveltekit-preload-data="hover" data-theme="cerberus">
<div>%sveltekit.body%</div>
</body>

</html>
<body data-sveltekit-preload-data="hover" data-theme="cerberus">
<div>%sveltekit.body%</div>
</body>
</html>
2 changes: 1 addition & 1 deletion packages/skeleton-svelte/src/lib/components/Accordion/Accordion.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
children,
iconOpen,
iconClosed
} = $props<AccordionProps>();
}: AccordionProps = $props();

// Context
setContext('selected', new State<string[]>([]));
Expand Down
2 changes: 1 addition & 1 deletion packages/skeleton-svelte/src/lib/components/Accordion/AccordionItem.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
control,
controlLead,
panel
} = $props<AccordionItemProps>();
}: AccordionItemProps = $props();

// Context
const selected = getContext<State<string[]>>('selected');
Expand Down
2 changes: 1 addition & 1 deletion packages/skeleton-svelte/src/lib/components/AppBar/AppBar.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
lead,
trail,
headline
} = $props<AppBarProps>();
}: AppBarProps = $props();
</script>

<!-- @component A header element for the top of a page layout. -->
Expand Down
2 changes: 1 addition & 1 deletion packages/skeleton-svelte/src/lib/components/Avatar/Avatar.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
imageClasses = '',
// Snippets
children
} = $props<AvatarProps>();
}: AvatarProps = $props();
</script>

<!-- @component An image with a fallback for representing the user. -->
Expand Down
92 changes: 92 additions & 0 deletions packages/skeleton-svelte/src/lib/components/Tab/Tabs.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
<script lang="ts">
import { setContext } from 'svelte';
import type { TabsProps } from './types.js';

let {
// A11Y
listLabelledBy = '',
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
panelLabelledBy = '',
// Root
base = '',
spaceY = 'space-y-4',
classes = '',
// Tab list
listBase = 'flex overflow-x-auto hide-scrollbar',
listSpaceX = 'space-x-4',
listJustify = 'justify-start',
listBorder = 'border-b border-surface-50-950',
listClasses = '',
// Tab panel
panelBase = '',
panelClasses = '',
// Tab Controls
controlsBase = 'flex',
controlsText = 'hover:text-primary-600-400',
controlsJustify = 'justify-center',
controlsActive = 'border-b-2 border-surface-950-50 text-surface-950-50',
controlsInactive = 'text-surface-600-400 fill-surface-600-400',
controlsBackground = '',
controlsPadding = 'px-4 py-2',
controlsRounded = '',
controlsSpacingY = 'space-y-1',
controlsCursor = 'cursor-pointer',
controlsClasses = '',
// Events
onclick = () => {},
onkeypress = () => {},
onkeydown = () => {},
onkeyup = () => {},
onchange = () => {},
// Snippets
list,
panel
}: TabsProps = $props();

setContext('base', controlsBase);
setContext('text', controlsText);
setContext('justify', controlsJustify);
setContext('active', controlsActive);
setContext('inactive', controlsInactive);
setContext('background', controlsBackground);
setContext('padding', controlsPadding);
setContext('rounded', controlsRounded);
setContext('spacingY', controlsSpacingY);
setContext('cursor', controlsCursor);
setContext('classes', controlsClasses);
setContext('onclick', onclick);
setContext('onkeypress', onkeypress);
setContext('onkeydown', onkeydown);
setContext('onkeyup', onkeyup);
setContext('onchange', onchange);
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved

let panelElem: HTMLDivElement | undefined = $state(undefined);
let panelTabIndex = $state(0);
// if the tabpanel doesn't contain focusable element, it will be focusable following the ARIA guidelines.
endigo9740 marked this conversation as resolved.
Show resolved Hide resolved
$effect(() => {
if (!panelElem) return;
const hasFocusableElements =
panelElem.querySelectorAll('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])').length > 0;
panelTabIndex = hasFocusableElements ? -1 : 0;
});
</script>

<!-- @component A Tab parent component. -->

<div class="{base} {spaceY} {classes}" data-testid="tab-group">
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
{#if list}
<ul class="{listBase} {listSpaceX} {listJustify} {listBorder} {listClasses}" role="tablist" aria-labelledby={listLabelledBy}>
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
{@render list()}
</ul>
{/if}
{#if panel}
<div
bind:this={panelElem}
class="{panelBase} {panelClasses}"
role="tabpanel"
aria-labelledby={panelLabelledBy}
tabindex={panelTabIndex}
>
{@render panel()}
</div>
{/if}
</div>
126 changes: 126 additions & 0 deletions packages/skeleton-svelte/src/lib/components/Tab/TabsControl.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
<script lang="ts">
import { getContext } from 'svelte';
import type { TabsControlProps } from './types.js';

let {
name,
group,
title = '',
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
// A11y
controls = '',
// Root
base = getContext<string>('base'),
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
text = getContext<string>('text'),
justify = getContext<string>('justify'),
active = getContext<string>('active'),
inactive = getContext<string>('inactive'),
background = getContext<string>('background'),
padding = getContext<string>('padding'),
rounded = getContext<string>('rounded'),
spacingY = getContext<string>('spacingY'),
cursor = getContext<string>('cursor'),
classes = getContext<string>('classes'),
// Tab
tabBase = '',
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed we have both tabX and contentX props. The label serves a functional purpose only, it should not be styled. I'd suggest moving these styles up/down to the parent/children elements surrounding the <label> element.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

reference #2541 (comment)

tabRounded = 'rounded-container',
tabOutline = 'focus:outline focus:outline-primary-600-400 focus:outline-offset-4',
tabClasses = '',
// Tab Content
contentBase = 'flex',
contentSpaceX = 'space-x-1',
contentClasses = '',
// Events
onclick = getContext<(event: MouseEvent) => {}>('onclick'),
onkeypress = getContext<(event: KeyboardEvent) => {}>('onkeypress'),
onkeydown = getContext<(event: KeyboardEvent) => {}>('onkeydown'),
onkeyup = getContext<(event: KeyboardEvent) => {}>('onkeyup'),
onchange = getContext<(group: string) => {}>('onchange'),
// Snippets
children
}: TabsControlProps = $props();

const selected = $derived(group === name);
const rxActive = $derived(selected ? active : inactive);

let elemInput: HTMLInputElement;

function onTabKeyDown(event: KeyboardEvent) {
onkeydown(event);
if (!['Enter', 'Space', 'ArrowRight', 'ArrowLeft', 'Home', 'End'].includes(event.code)) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need to create an allowlist of codes. Just handle the code if it's appropriate.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

this will simplify the code a lot... I made some changes but I find it necessary for multiple keys that share some functionalities (we don't have to duplicate code if we use this style of escaping first).


event.preventDefault();
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved

if (['Enter', 'Space'].includes(event.code)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

See above, this should be unnecessary if implemented properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

One again, I'd say let's disable and rethink our keyboard interaction strategy. I'll save my comments for the rest of this section.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

this is a common pattern to group keys functionality, using a switch for tow cases with the same behavior will make the code harder to read.

elemInput.click();
return;
}

const currTab = elemInput.closest('[role="tab"]');
if (!currTab) return;

const tabList = elemInput.closest('[role="tablist"]');
if (!tabList) return;

const isRTL = getComputedStyle(tabList).direction === 'rtl';
const tabs = Array.from(tabList.querySelectorAll('[role="tab"]'));
const currIndex = tabs.indexOf(currTab);

let nextIndex = -1;
switch (event.code) {
case 'ArrowRight':
if (isRTL) {
nextIndex = currIndex - 1 < 0 ? tabs.length - 1 : currIndex - 1;
} else {
nextIndex = currIndex + 1 >= tabs.length ? 0 : currIndex + 1;
}
break;
case 'ArrowLeft':
if (isRTL) {
nextIndex = currIndex + 1 >= tabs.length ? 0 : currIndex + 1;
} else {
nextIndex = currIndex - 1 < 0 ? tabs.length - 1 : currIndex - 1;
}
break;
case 'Home':
nextIndex = 0;
break;
case 'End':
nextIndex = tabs.length - 1;
break;
}

if (nextIndex < 0) return;

const nextTab = tabs![nextIndex!];
const nextTabInput = nextTab?.querySelector('input');
if (nextTabInput) {
nextTabInput.click();
(nextTab as HTMLDivElement).focus();
}
}
</script>

<!-- @component A Tab Control component. -->

<label class="{base} {text} {justify} {rxActive} {background} {padding} {rounded} {spacingY} {cursor} {classes}" {title}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Echoing: avoid style duplication between the label and wrapping element within. Pick one or the other. This adds extra overhead for users to understand.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

duplicating the styles was necessary because we have tow different styling levels, one for the line under the active tab (full cell style) and one for the content (outline styles).
I would be happy to change it if you have a suggestion on how to solve some of the issues that comes from combining both, for example the rounding of the outline and the rounding of the line under the active tab.

<li
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
aria-controls={controls}
tabindex={selected ? 0 : -1}
aria-selected={selected}
data-testid="tabs-control"
role="tab"
class="{tabBase} {tabRounded} {tabOutline} {tabClasses}"
onkeydown={onTabKeyDown}
{onkeypress}
{onkeyup}
>
<div class="h-0 w-0 flex-none overflow-hidden">
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
<input bind:this={elemInput} type="radio" bind:group {name} value={name} onchange={() => onchange(group)} {onclick} tabindex="-1" />
endigo9740 marked this conversation as resolved.
Show resolved Hide resolved
</div>
{#if children}
<div class="{contentBase} {contentSpaceX} {contentClasses}">
endigo9740 marked this conversation as resolved.
Show resolved Hide resolved
{@render children()}
</div>
{/if}
</li>
</label>
21 changes: 21 additions & 0 deletions packages/skeleton-svelte/src/lib/components/Tab/TabsPanel.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<script lang="ts">
import type { TabsPanelProps } from './types.js';

let {
value,
group,
// Root
base = '',
endigo9740 marked this conversation as resolved.
Show resolved Hide resolved
classes = '',
// Snippets
children
}: TabsPanelProps = $props();
</script>

<!-- @component A Tab Panel component. -->

{#if value === group && children}
<section class="{base} {classes}">
Mahmoud-zino marked this conversation as resolved.
Show resolved Hide resolved
{@render children()}
</section>
{/if}
5 changes: 5 additions & 0 deletions packages/skeleton-svelte/src/lib/components/Tab/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Tabs from './Tabs.svelte';
import Control from './TabsControl.svelte';
import Panel from './TabsPanel.svelte';

export default Object.assign(Tabs, { Control, Panel });