Skip to content

Shortcut improvements #1573

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

Merged
merged 27 commits into from
Jan 8, 2025
Merged

Shortcut improvements #1573

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
84d0508
Added shortcuts (with tooltip) to the pagination
samejr Dec 13, 2024
75ec976
WIP fixing the disabled hover state
samejr Dec 13, 2024
f341887
Styled the tooltip
samejr Dec 16, 2024
c5c1d1d
Added a shortcuts panel to the help menu
samejr Dec 16, 2024
3d10901
Adds new shortcut to list
samejr Dec 16, 2024
afe13a8
Changes “meta” for “mod”
samejr Dec 16, 2024
97e610a
Adds more shortcuts to the list
samejr Dec 16, 2024
747caa6
Adding shortcut to open the shortcuts panel
samejr Dec 17, 2024
13232e2
tweak gap between shortcut letters
samejr Dec 17, 2024
2ac815f
button component now has icon spacing adjustment (for lucide icons)
samejr Dec 17, 2024
bef4432
Fixed some ilegal markup
samejr Dec 17, 2024
113d2e4
Pagination uses disabled prop rather than a disabled wrapper
samejr Dec 17, 2024
72d1381
Improved the Switch styles
samejr Dec 17, 2024
277a69f
Merge branch 'main' into shortcut-improvements
samejr Dec 17, 2024
0958943
Makes the shortcut modifier optional
samejr Dec 17, 2024
2a50b09
Added new icon based shortcut keys for mac and win
samejr Dec 17, 2024
225eae8
Updated PC modifier shortcuts
samejr Dec 17, 2024
3e27fe9
Adds a new windows key icon
samejr Dec 18, 2024
b937cce
Allows variants and react nodes to be used as the modifier key
samejr Dec 18, 2024
e4446b2
Adds more shortcuts to the storybook
samejr Dec 18, 2024
4dcf218
Adds missing focus-visible styles to the pagination
samejr Dec 18, 2024
ededd0d
Removed test modifier keys
samejr Dec 18, 2024
f8e97c6
number style is tabular
samejr Dec 18, 2024
ba16fcc
Merge branch 'main' into shortcut-improvements
samejr Dec 18, 2024
fe31906
Update apps/webapp/app/components/primitives/ShortcutKey.tsx
matt-aitken Dec 30, 2024
f189fc0
Tooltip now just 1 prop on the button component
samejr Dec 31, 2024
a1ec1c1
Merge branch 'main' into shortcut-improvements
samejr Jan 3, 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
2 changes: 1 addition & 1 deletion apps/webapp/app/components/ErrorDisplay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function ErrorDisplay({ title, message, button }: DisplayOptionsProps) {
{message && <Paragraph>{message}</Paragraph>}
<LinkButton
to={button ? button.to : "/"}
shortcut={{ modifiers: ["meta"], key: "g" }}
shortcut={{ modifiers: ["mod"], key: "g" }}
variant="primary/medium"
LeadingIcon={HomeIcon}
>
Expand Down
12 changes: 8 additions & 4 deletions apps/webapp/app/components/ListPagination.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ function NextButton({ cursor }: { cursor?: string }) {
trailingIconClassName="text-text-dimmed"
className={cn(
"flex items-center",
!path &&
"cursor-not-allowed opacity-50 group-hover:bg-transparent group-hover:text-text-dimmed"
!path && "cursor-not-allowed opacity-50 group-hover/button:bg-transparent"
)}
onClick={(e) => !path && e.preventDefault()}
shortcut={{ key: "k" }}
showTooltip
tooltipDescription="Next"
>
Next
</LinkButton>
Expand All @@ -51,10 +53,12 @@ function PreviousButton({ cursor }: { cursor?: string }) {
leadingIconClassName="text-text-dimmed"
className={cn(
"flex items-center",
!path &&
"cursor-not-allowed opacity-50 group-hover:bg-transparent group-hover:text-text-dimmed"
!path && "cursor-not-allowed opacity-50 group-hover/button:bg-transparent"
)}
onClick={(e) => !path && e.preventDefault()}
shortcut={{ key: "j" }}
showTooltip
tooltipDescription="Previous"
>
Prev
</LinkButton>
Expand Down
148 changes: 148 additions & 0 deletions apps/webapp/app/components/Shortcuts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
import { Keyboard } from "lucide-react";
import { Header3 } from "./primitives/Headers";
import { Paragraph } from "./primitives/Paragraph";
import {
Sheet,
SheetContent,
SheetDescription,
SheetHeader,
SheetTitle,
SheetTrigger,
} from "./primitives/SheetV3";
import { ShortcutKey } from "./primitives/ShortcutKey";
import { Button } from "./primitives/Buttons";

export function Shortcuts() {
return (
<Sheet>
<SheetTrigger asChild>
<Button
variant="small-menu-item"
LeadingIcon={Keyboard}
leadingIconClassName="text-blue-500"
data-action="shortcuts"
fullWidth
textAlignLeft
shortcut={{ modifiers: ["shift"], key: "?" }}
className="gap-x-0 pl-0.5"
iconSpacing="gap-x-0.5"
>
Shortcuts
</Button>
</SheetTrigger>
<SheetContent>
<SheetHeader>
<SheetTitle>
<div className="flex items-center gap-x-2">
<Keyboard className="size-5 text-indigo-500" />
<span className="font-sans text-base font-medium text-text-bright">
Keyboard shortcuts
</span>
</div>
</SheetTitle>
<div className="space-y-6 px-4 py-2">
<div className="space-y-3">
<Header3>General</Header3>
<Shortcut name="Close">
<ShortcutKey shortcut={{ key: "esc" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Confirm">
<ShortcutKey shortcut={{ key: "", modifiers: ["mod"] }} variant="medium/bright" />
<ShortcutKey shortcut={{ key: "enter" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Filter">
<ShortcutKey shortcut={{ key: "f" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Select filter">
<ShortcutKey shortcut={{ key: "1" }} variant="medium/bright" />
<Paragraph variant="small" className="ml-1">
to
</Paragraph>
<ShortcutKey shortcut={{ key: "9" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Previous page">
<ShortcutKey shortcut={{ key: "j" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Next page">
<ShortcutKey shortcut={{ key: "k" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Help & Feedback">
<ShortcutKey shortcut={{ key: "h" }} variant="medium/bright" />
</Shortcut>
</div>
<div className="space-y-3">
<Header3>Runs page</Header3>
<Shortcut name="Bulk action: Cancel runs">
<ShortcutKey shortcut={{ key: "c" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Bulk action: Replay runs">
<ShortcutKey shortcut={{ key: "r" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Bulk action: Clear selection">
<ShortcutKey shortcut={{ key: "esc" }} variant="medium/bright" />
</Shortcut>
</div>
<div className="space-y-3">
<Header3>Run page</Header3>
<Shortcut name="Replay run">
<ShortcutKey shortcut={{ key: "r" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Overview">
<ShortcutKey shortcut={{ key: "o" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Details">
<ShortcutKey shortcut={{ key: "d" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Context">
<ShortcutKey shortcut={{ key: "c" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Metadata">
<ShortcutKey shortcut={{ key: "m" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Navigate">
<ShortcutKey shortcut={{ key: "arrowup" }} variant="medium/bright" />
<ShortcutKey shortcut={{ key: "arrowdown" }} variant="medium/bright" />
<ShortcutKey shortcut={{ key: "arrowleft" }} variant="medium/bright" />
<ShortcutKey shortcut={{ key: "arrowright" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Expand all">
<ShortcutKey shortcut={{ key: "e" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Collapse all">
<ShortcutKey shortcut={{ key: "w" }} variant="medium/bright" />
</Shortcut>
<Shortcut name="Toggle level">
<ShortcutKey shortcut={{ key: "0" }} variant="medium/bright" />
<Paragraph variant="small" className="ml-1">
to
</Paragraph>
<ShortcutKey shortcut={{ key: "9" }} variant="medium/bright" />
</Shortcut>
</div>
<div className="space-y-3">
<Header3>Schedules page</Header3>
<Shortcut name="New schedule">
<ShortcutKey shortcut={{ key: "n" }} variant="medium/bright" />
</Shortcut>
</div>
<div className="space-y-3">
<Header3>Alerts page</Header3>
<Shortcut name="New alert">
<ShortcutKey shortcut={{ key: "n" }} variant="medium/bright" />
</Shortcut>
</div>
</div>
</SheetHeader>
</SheetContent>
</Sheet>
);
}

function Shortcut({ children, name }: { children: React.ReactNode; name: string }) {
return (
<div className="flex items-center justify-between gap-x-2">
<span className="text-sm text-text-dimmed">{name}</span>
<span className="flex items-center gap-x-0.5">{children}</span>
</div>
);
}
3 changes: 2 additions & 1 deletion apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { Paragraph } from "../primitives/Paragraph";
import { Popover, PopoverContent, PopoverSideMenuTrigger } from "../primitives/Popover";
import { StepNumber } from "../primitives/StepNumber";
import { MenuCount, SideMenuItem } from "./SideMenuItem";

import { Shortcuts } from "../Shortcuts";
export function HelpAndFeedback() {
const [isHelpMenuOpen, setHelpMenuOpen] = useState(false);
const currentPlan = useCurrentPlan();
Expand Down Expand Up @@ -84,6 +84,7 @@ export function HelpAndFeedback() {
data-action="changelog"
target="_blank"
/>
<Shortcuts />
</div>
<div className="flex flex-col gap-1 p-1">
<Paragraph className="pb-1 pl-1.5 pt-1.5 text-xs">Need help?</Paragraph>
Expand Down
61 changes: 42 additions & 19 deletions apps/webapp/app/components/primitives/Buttons.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ShortcutDefinition, useShortcutKeys } from "~/hooks/useShortcutKeys";
import { cn } from "~/utils/cn";
import { IconNamesOrString, NamedIcon } from "./NamedIcon";
import { ShortcutKey } from "./ShortcutKey";
import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "./Tooltip";

const sizes = {
small: {
Expand Down Expand Up @@ -65,8 +66,7 @@ const theme = {
icon: "text-text-bright",
},
minimal: {
textColor:
"text-text-dimmed group-hover/button:text-text-bright transition group-disabled/button:text-text-dimmed/80",
textColor: "text-text-dimmed group-disabled/button:text-text-dimmed transition",
button:
"bg-transparent group-hover/button:bg-tertiary disabled:opacity-50 group-disabled/button:bg-transparent group-disabled/button:pointer-events-none",
shortcut:
Expand Down Expand Up @@ -173,8 +173,11 @@ export type ButtonContentPropsType = {
textAlignLeft?: boolean;
className?: string;
shortcut?: ShortcutDefinition;
showTooltip?: boolean;
variant: keyof typeof variant;
shortcutPosition?: "before-trailing-icon" | "after-trailing-icon";
tooltipDescription?: string;
iconSpacing?: string;
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure 'tooltipDescription' is provided when 'showTooltip' is true

When showTooltip is true, tooltipDescription is used to render the tooltip content. If tooltipDescription is undefined, it may result in displaying 'undefined' in the tooltip. Consider making tooltipDescription required when showTooltip is true to prevent this issue.

Apply this diff to update the type definition:

 export type ButtonContentPropsType = {
   // existing props
   showTooltip?: boolean;
-  tooltipDescription?: string;
+  tooltipDescription?: string;
 } & (
+  showTooltip extends true ? { tooltipDescription: string } : {}
 );

This change will enforce that tooltipDescription is provided whenever showTooltip is true.

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

Copy link
Member

Choose a reason for hiding this comment

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

@samejr same comment as above. Do we need both of these? Or should we just have tooltipDescription, which probably should just be called tooltip. And should probably be independent from a shortcut thinking about it, so you can have a tooltip without a shortcut.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of being a string, it should be a React Node

};

export function ButtonContent(props: ButtonContentPropsType) {
Expand All @@ -188,23 +191,35 @@ export function ButtonContent(props: ButtonContentPropsType) {
fullWidth,
textAlignLeft,
className,
showTooltip,
tooltipDescription,
iconSpacing,
} = props;
const variation = allVariants.variant[props.variant];

// Based on the size prop, we'll use the corresponding variant classnames
const btnClassName = cn(allVariants.$all, variation.button);
const iconClassName = variation.icon;
const iconSpacingClassName = variation.iconSpacing;
const shortcutClassName = variation.shortcut;
const textColorClassName = variation.textColor;

return (
const renderShortcutKey = () =>
shortcut && (
<ShortcutKey
className={cn(shortcutClassName)}
shortcut={shortcut}
variant={variation.shortcutVariant ?? "medium"}
/>
);

const buttonContent = (
<div className={cn("flex", fullWidth ? "" : "w-fit text-xxs", btnClassName, className)}>
<div
className={cn(
textAlignLeft ? "text-left" : "justify-center",
"flex w-full items-center",
iconSpacingClassName
iconSpacingClassName,
iconSpacing
)}
>
{LeadingIcon &&
Expand Down Expand Up @@ -238,13 +253,10 @@ export function ButtonContent(props: ButtonContentPropsType) {
<>{text}</>
))}

{shortcut && props.shortcutPosition === "before-trailing-icon" && (
<ShortcutKey
className={cn(shortcutClassName)}
shortcut={shortcut}
variant={variation.shortcutVariant ?? "medium"}
/>
)}
{shortcut &&
!showTooltip &&
props.shortcutPosition === "before-trailing-icon" &&
renderShortcutKey()}

{TrailingIcon &&
(typeof TrailingIcon === "string" ? (
Expand All @@ -269,16 +281,27 @@ export function ButtonContent(props: ButtonContentPropsType) {
))}

{shortcut &&
(!props.shortcutPosition || props.shortcutPosition === "after-trailing-icon") && (
<ShortcutKey
className={cn(shortcutClassName)}
shortcut={shortcut}
variant={variation.shortcutVariant ?? "medium"}
/>
)}
!showTooltip &&
(!props.shortcutPosition || props.shortcutPosition === "after-trailing-icon") &&
renderShortcutKey()}
</div>
</div>
);

if (shortcut && showTooltip) {
return (
<TooltipProvider>
<Tooltip>
<TooltipTrigger asChild>{buttonContent}</TooltipTrigger>
<TooltipContent className="text-dimmed flex items-center gap-3 py-1.5 pl-2.5 pr-3 text-xs">
{tooltipDescription} {renderShortcutKey()}
</TooltipContent>
</Tooltip>
</TooltipProvider>
);
}
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle undefined 'tooltipDescription' in tooltip rendering

In the ButtonContent component, when rendering the tooltip content, tooltipDescription may be undefined if not provided. To prevent displaying 'undefined' in the tooltip, consider providing a default value or conditionally rendering the tooltip content.

Modify the tooltip rendering logic as follows:

 <TooltipContent className="text-dimmed flex items-center gap-3 py-1.5 pl-2.5 pr-3 text-xs">
-  {tooltipDescription} {renderShortcutKey()}
+  {tooltipDescription && <span>{tooltipDescription}</span>} {renderShortcutKey()}
 </TooltipContent>

This change ensures that the tooltip content is only rendered when tooltipDescription is defined.

📝 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
if (shortcut && showTooltip) {
return (
<TooltipProvider>
<Tooltip>
<TooltipTrigger asChild>{buttonContent}</TooltipTrigger>
<TooltipContent className="text-dimmed flex items-center gap-3 py-1.5 pl-2.5 pr-3 text-xs">
{tooltipDescription} {renderShortcutKey()}
</TooltipContent>
</Tooltip>
</TooltipProvider>
);
}
if (shortcut && showTooltip) {
return (
<TooltipProvider>
<Tooltip>
<TooltipTrigger asChild>{buttonContent}</TooltipTrigger>
<TooltipContent className="text-dimmed flex items-center gap-3 py-1.5 pl-2.5 pr-3 text-xs">
{tooltipDescription && <span>{tooltipDescription}</span>} {renderShortcutKey()}
</TooltipContent>
</Tooltip>
</TooltipProvider>
);
}

Copy link
Member

Choose a reason for hiding this comment

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

@samejr this is a good point. If tooltipDescription isn't set, what do you want to happen? Is it compulsory? In which case we should only have tooltipDescription and remove showTooltip.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


return buttonContent;
}

type ButtonPropsType = Pick<
Expand Down
19 changes: 0 additions & 19 deletions apps/webapp/app/components/primitives/LinkWithDisabled.tsx
View file
Open in desktop

This file was deleted.

Loading