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

⌨️ Keyboard shortcuts/navigation for create and upload #736

Merged
merged 3 commits into from
Dec 19, 2024
Merged

⌨️ Keyboard shortcuts/navigation for create and upload #736

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1969b3b
feat: keyboard shortcuts
MontaGhanmy Nov 20, 2024
a4cc7ed
ref: feedback
MontaGhanmy Nov 27, 2024
e3e0e6e
merge/resolve conflict with main
MontaGhanmy Dec 19, 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
57 changes: 53 additions & 4 deletions tdrive/frontend/src/app/atoms/button/button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import React from 'react';
import React, { useEffect } from 'react';
import _ from 'lodash';
import { addShortcut, removeShortcut } from 'app/features/global/services/shortcut-service';

export type ButtonTheme = 'primary' | 'secondary' | 'danger' | 'default' | 'outline' | 'dark' | 'white' | 'green';

Expand All @@ -12,6 +13,7 @@ interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {
loading?: boolean;
disabled?: boolean;
children?: React.ReactNode;
shortcut?: string;
}

export const Button = (props: ButtonProps) => {
Expand Down Expand Up @@ -42,9 +44,7 @@ export const Button = (props: ButtonProps) => {
className =
'text-zinc-300 border-0 bg-zinc-900 hover:bg-zinc-800 hover:text-white active:bg-zinc-900';

if (props.theme === 'green')
className =
'text-zinc-300 border-0 bg-green-700';
if (props.theme === 'green') className = 'text-zinc-300 border-0 bg-green-700';

if (disabled) className += ' opacity-50 pointer-events-none';

Expand All @@ -58,6 +58,46 @@ export const Button = (props: ButtonProps) => {
else className = className + ' w-9 !p-0 justify-center';
}

// translate shortcuts
const translateDynamicShortcut = (shortcut: string): string =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should probably be in app/features/global/services/shortcut-service

shortcut.startsWith('CmdOrCtrl+Shift')
? `⇧⌘${shortcut.replace('CmdOrCtrl+Shift+', '')}`
Copy link
Contributor

Choose a reason for hiding this comment

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

this symbol means nothing to non mac users =/ i think we should use https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform to show control if it isn't mac

: shortcut.startsWith('CmdOrCtrl')
? `⌘${shortcut.replace('CmdOrCtrl+', '')}`
: shortcut;

if (props.shortcut) {
useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the if (shortcut) should be inside the useEffect callback, because useEffect and friends need to always be called in the same exact order and sequence

const handler = (event?: KeyboardEvent) => {
// Disable default behavior for the shortcut
if (event) {
event.preventDefault();
event.stopPropagation();
}
props.onClick && props.onClick({} as any);
};

const shortcut = props.shortcut || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

this is inside if (props.shortcut) so it's always truthy here


// Add shortcut with default behavior prevention
addShortcut({
shortcut,
handler: event => {
handler(event as KeyboardEvent);
},
});

return () => {
removeShortcut({
shortcut,
handler: event => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function instance will not be the same as the one when added. If the event source tests for the handler it will fail, if it doesn't, this is useless

handler(event as KeyboardEvent);
},
});
};
}, [props.onClick, props.shortcut]);
}

return (
<button
type="button"
Expand Down Expand Up @@ -103,6 +143,15 @@ export const Button = (props: ButtonProps) => {
/>
)}
{props.children}
{props.shortcut && props.theme !== 'white' && (
<span
className={`ml-2 text-xs ${
props.theme === 'primary' ? 'text-gray-200' : 'text-gray-400'
}`}
>
{translateDynamicShortcut(props.shortcut)}
</span>
)}
</button>
);
};
9 changes: 6 additions & 3 deletions tdrive/frontend/src/app/atoms/modal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Fragment, ReactNode, useCallback, useEffect, useState } from 'react';
import { atom, useRecoilState } from 'recoil';
import { DismissIcon } from '../icons-colored';
import { Title } from '../text';
import { Button } from '@atoms/button/button';

const ModalsCountState = atom({
key: 'ModalsState',
Expand Down Expand Up @@ -126,13 +127,15 @@ export const Modal = (props: {
>
{props.closable !== false && (
<div className="absolute top-0 right-0 pt-4 pr-4">
<button
<Button
type="button"
className="hover:opacity-75 focus:outline-none "
onClick={() => props.onClose && props.onClose()}
shortcut="esc"
theme="white"
>
{props.closeIcon ? props.closeIcon : <DismissIcon />}
</button>
</Button>
</div>
)}
{didOpenOnce && props.children}
Expand Down Expand Up @@ -177,7 +180,7 @@ export const ModalContent = (props: {
}
>
<Title className="pr-8 overflow-hidden text-ellipsis">{props.title}</Title>
<div className="mt-2">
<div className="mt-4">
<p className="text-sm text-gray-500">{props.text || ''}</p>
</div>
</div>
Expand Down
11 changes: 10 additions & 1 deletion tdrive/frontend/src/app/views/client/body/drive/modals/create/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,19 @@ export const CreateModal = ({
};

const CreateModalOption = (props: { icon: ReactNode; text: string; onClick: () => void }) => {
// on press enter
const handleKeyPress = (e: React.KeyboardEvent<HTMLDivElement>) => {
if (e.key === 'Enter') {
props.onClick();
}
};

return (
<div
onClick={props.onClick}
className="flex flex-row p-4 dark:bg-zinc-900 dark:text-white bg-zinc-100 hover:bg-opacity-75 cursor-pointer rounded-md m-2"
className="flex flex-row p-4 dark:bg-zinc-900 dark:text-white bg-zinc-100 hover:bg-opacity-75 cursor-pointer rounded-md m-2 focus:bg-zinc-800 dark:focus:bg-zinc-800 outline-none focus:border-none"
tabIndex={0}
Copy link
Contributor

Choose a reason for hiding this comment

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

why the {} and not just 0 ? is this required at all, we're not really doing the tab indexing stuff anywhere else ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tabIndex is a number that's why. It is required for tab to work/navigate the modal items.

Copy link
Contributor

Choose a reason for hiding this comment

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

html dom attribute values are always strings anyway, see https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute . what's happening here is just making react type it first and maybe not guess its a constant it could optimise out

onKeyUp={handleKeyPress}
>
<div className="flex items-center justify-center">{props.icon}</div>
<div className="grow flex items-center ml-2">
Expand Down
11 changes: 10 additions & 1 deletion tdrive/frontend/src/app/views/client/body/drive/modals/upload/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,19 @@ export const UploadModal = ({
};

const CreateModalOption = (props: { icon: ReactNode; text: string; onClick: () => void }) => {
// on press enter
const handleKeyPress = (e: React.KeyboardEvent<HTMLDivElement>) => {
if (e.key === 'Enter') {
props.onClick();
}
};

return (
<div
onClick={props.onClick}
className="flex flex-row p-4 dark:bg-zinc-900 dark:text-white bg-zinc-100 hover:bg-opacity-75 cursor-pointer rounded-md m-2"
className="flex flex-row p-4 dark:bg-zinc-900 dark:text-white bg-zinc-100 hover:bg-opacity-75 cursor-pointer rounded-md m-2 focus:bg-zinc-800 dark:focus:bg-zinc-800 outline-none focus:border-none"
tabIndex={0}
onKeyUp={handleKeyPress}
>
<div className="flex items-center justify-center">{props.icon}</div>
<div className="grow flex items-center ml-2">
Expand Down
2 changes: 2 additions & 0 deletions tdrive/frontend/src/app/views/client/side-bar/actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ export default () => {

<Button
onClick={() => uploadItemModal()}
shortcut='CmdOrCtrl+U'
Copy link
Contributor

Choose a reason for hiding this comment

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

is the modifier necessary ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

size="lg"
theme="primary"
className="w-full mb-2 justify-center"
Expand All @@ -171,6 +172,7 @@ export default () => {
</Button>
<Button
onClick={() => openItemModal()}
shortcut='CmdOrCtrl+C'
size="lg"
theme="secondary"
className="w-full mb-2 justify-center"
Expand Down