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

tabs component #1557

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

tabs component #1557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebcb357
init
adred Dec 18, 2024
e5e6e7f
tabs
adred Dec 18, 2024
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
39 changes: 39 additions & 0 deletions frontend/app/element/tabs.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2024, Command Line Inc.
// SPDX-License-Identifier: Apache-2.0

.tabs-container {
display: flex;
flex-direction: column;
font-family: Arial, sans-serif;
font-size: 12px;

.tabs-list {
display: flex;
gap: 16px;
border: 1px solid rgb(from var(--main-text-color) r g b / 15%);
border-radius: 7px;
padding: 4px;
background: rgb(from var(--block-bg-color) r g b / 70%);

.tab-item {
display: flex;
height: 24px;
padding: 0px 8px;
justify-content: center;
align-items: center;
cursor: pointer;
border-radius: 4px;
color: var(--main-text-color);
border: 1px solid transparent;

&:hover {
background-color: rgb(from var(--main-bg-color) r g b / 60%);
}

&.active {
border: 1px solid var(--success-color);
background: rgb(from var(--success-color) r g b / 15%);
}
}
}
}
47 changes: 47 additions & 0 deletions frontend/app/element/tabs.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright 2024, Command Line Inc.
// SPDX-License-Identifier: Apache-2.0

import type { Meta, StoryObj } from "@storybook/react";
import { Tabs } from "./tabs";

const meta: Meta<typeof Tabs> = {
title: "Elements/Tabs",
component: Tabs,
argTypes: {},
};

export default meta;

type Story = StoryObj<typeof Tabs>;

export const DefaultTabs: Story = {
render: () => {
const tabs = [
{ label: "Node 1", onClick: () => console.log("Node 1 Clicked") },
{ label: "Node 2", onClick: () => console.log("Node 2 Clicked") },
{ label: "Node 3", onClick: () => console.log("Node 3 Clicked") },
];

return (
<div style={{ padding: "20px", backgroundColor: "#000", color: "#fff" }}>
<Tabs tabs={tabs} />
</div>
);
},
};

export const TabsWithAlerts: Story = {
render: () => {
const tabs = [
{ label: "Node 1", onClick: () => alert("Node 1 Clicked") },
{ label: "Node 2", onClick: () => alert("Node 2 Clicked") },
{ label: "Node 3", onClick: () => alert("Node 3 Clicked") },
];

return (
<div style={{ padding: "20px", backgroundColor: "#000", color: "#fff" }}>
<Tabs tabs={tabs} />
</div>
);
},
};
Comment on lines +33 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace alert() with a more user-friendly notification system.

Using alert() is not recommended as it:

  1. Blocks the UI
  2. Provides a poor user experience
  3. Doesn't represent real-world usage

Consider using a toast notification or a more elegant feedback mechanism.

export const TabsWithAlerts: Story = {
    render: () => {
        const tabs = [
-           { label: "Node 1", onClick: () => alert("Node 1 Clicked") },
-           { label: "Node 2", onClick: () => alert("Node 2 Clicked") },
-           { label: "Node 3", onClick: () => alert("Node 3 Clicked") },
+           { 
+               label: "Node 1", 
+               onClick: () => {
+                   // Use a toast notification or custom feedback component
+               } 
+           },
            // Similar changes for other tabs
        ];

        return (
-           <div style={{ padding: "20px", backgroundColor: "#000", color: "#fff" }}>
+           <div className="story-container">
                <Tabs tabs={tabs} />
            </div>
        );
    },
};

Committable suggestion skipped: line range outside the PR's diff.

55 changes: 55 additions & 0 deletions frontend/app/element/tabs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2024, Command Line Inc.
// SPDX-License-Identifier: Apache-2.0

import React, { useState } from "react";
import "./tabs.scss";

type Tab = {
label: string;
onClick: () => void;
};

type TabsProps = {
tabs: Tab[];
};
Comment on lines +7 to +14
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance type definitions for better accessibility and flexibility.

The current type definitions could be improved to support:

  1. ARIA labels for better accessibility
  2. Optional IDs for tabs
  3. Disabled state for tabs
type Tab = {
    label: string;
+   id?: string;
+   ariaLabel?: string;
+   disabled?: boolean;
    onClick: () => void;
};

type TabsProps = {
    tabs: Tab[];
+   ariaLabel?: string;
+   defaultActiveIndex?: number;
};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type Tab = {
label: string;
onClick: () => void;
};
type TabsProps = {
tabs: Tab[];
};
type Tab = {
label: string;
id?: string;
ariaLabel?: string;
disabled?: boolean;
onClick: () => void;
};
type TabsProps = {
tabs: Tab[];
ariaLabel?: string;
defaultActiveIndex?: number;
};


const Tabs: React.FC<TabsProps> = ({ tabs }) => {
const [activeIndex, setActiveIndex] = useState(0);

const handleKeyDown = (event: React.KeyboardEvent<HTMLDivElement>, index: number) => {
if (event.key === "ArrowRight") {
setActiveIndex((prevIndex) => (prevIndex + 1) % tabs.length);
} else if (event.key === "ArrowLeft") {
setActiveIndex((prevIndex) => (prevIndex - 1 + tabs.length) % tabs.length);
} else if (event.key === "Enter" || event.key === " ") {
event.preventDefault();
tabs[index].onClick();
setActiveIndex(index);
}
};
Comment on lines +19 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance keyboard navigation support.

The keyboard handler should:

  1. Support Home/End keys for first/last tab navigation
  2. Prevent default on arrow keys to avoid page scrolling
  3. Skip disabled tabs if implemented
const handleKeyDown = (event: React.KeyboardEvent<HTMLDivElement>, index: number) => {
+   const { key } = event;
+   let newIndex = index;

-   if (event.key === "ArrowRight") {
+   if (key === "ArrowRight" || key === "ArrowLeft") {
        event.preventDefault();
-       setActiveIndex((prevIndex) => (prevIndex + 1) % tabs.length);
+       newIndex = key === "ArrowRight" 
+           ? (index + 1) % tabs.length
+           : (index - 1 + tabs.length) % tabs.length;
-   } else if (event.key === "ArrowLeft") {
-       setActiveIndex((prevIndex) => (prevIndex - 1 + tabs.length) % tabs.length);
-   } else if (event.key === "Enter" || event.key === " ") {
+   } else if (key === "Home" || key === "End") {
+       event.preventDefault();
+       newIndex = key === "Home" ? 0 : tabs.length - 1;
+   } else if (key === "Enter" || key === " ") {
        event.preventDefault();
-       tabs[index].onClick();
-       setActiveIndex(index);
+       if (!tabs[index].disabled) {
+           tabs[index].onClick();
+           setActiveIndex(index);
+       }
+       return;
    }
+   
+   // Find next non-disabled tab
+   while (tabs[newIndex]?.disabled) {
+       newIndex = key === "ArrowRight" || key === "Home"
+           ? (newIndex + 1) % tabs.length
+           : (newIndex - 1 + tabs.length) % tabs.length;
+   }
+   
+   setActiveIndex(newIndex);
};

Committable suggestion skipped: line range outside the PR's diff.


return (
<div className="tabs-container">
<div className="tabs-list" role="tablist">
{tabs.map((tab, index) => (
<div
key={index}
role="tab"
tabIndex={activeIndex === index ? 0 : -1}
aria-selected={activeIndex === index}
className={`tab-item ${activeIndex === index ? "active" : ""}`}
onClick={() => {
tab.onClick();
setActiveIndex(index);
}}
onKeyDown={(e) => handleKeyDown(e, index)}
>
{tab.label}
</div>
))}
</div>
</div>
);
};
Comment on lines +31 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance accessibility and semantic markup.

The render method should:

  1. Use <button> elements for better semantics
  2. Add additional ARIA attributes
  3. Support disabled state styling
return (
-   <div className="tabs-container">
+   <div 
+       className="tabs-container"
+       aria-label={ariaLabel}
+   >
        <div className="tabs-list" role="tablist">
            {tabs.map((tab, index) => (
-               <div
+               <button
                    key={index}
+                   id={tab.id}
                    role="tab"
                    tabIndex={activeIndex === index ? 0 : -1}
                    aria-selected={activeIndex === index}
+                   aria-label={tab.ariaLabel}
+                   aria-controls={`${tab.id}-panel`}
+                   disabled={tab.disabled}
                    className={`tab-item ${
                        activeIndex === index ? "active" : ""
+                       tab.disabled ? "disabled" : ""
                    }`}
                    onClick={() => {
+                       if (tab.disabled) return;
                        tab.onClick();
                        setActiveIndex(index);
                    }}
                    onKeyDown={(e) => handleKeyDown(e, index)}
                >
                    {tab.label}
-               </div>
+               </button>
            ))}
        </div>
    </div>
);

Committable suggestion skipped: line range outside the PR's diff.


export { Tabs };
Loading