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

Feat: QR code #90

Merged
merged 1 commit into from
Sep 25, 2024
Merged

Feat: QR code #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/app/components/StyledDialogActions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { DialogActions, styled } from '@mui/material';

export const StyledDialogActions = styled(DialogActions)(() => ({
paddingTop: '15px',
paddingRight: '25px',
paddingBottom: '15px',
backgroundImage: 'linear-gradient(to top, hsla(0, 0%, 90%, .05), #e6e6e6)',
}));
Comment on lines +3 to +8
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the following improvements for consistency and maintainability:

  1. The padding is inconsistent. Consider adding left padding for symmetry:
   paddingTop: '15px',
   paddingRight: '25px',
   paddingBottom: '15px',
+  paddingLeft: '25px',
  1. The background gradient mixes HSL and hex color formats. For consistency, consider using a single format:
-  backgroundImage: 'linear-gradient(to top, hsla(0, 0%, 90%, .05), #e6e6e6)',
+  backgroundImage: 'linear-gradient(to top, hsla(0, 0%, 90%, 0.05), hsla(0, 0%, 90%, 1))',
  1. To improve maintainability, consider using theme variables for colors and sizes:
-export const StyledDialogActions = styled(DialogActions)(() => ({
+export const StyledDialogActions = styled(DialogActions)(({ theme }) => ({
-  paddingTop: '15px',
-  paddingRight: '25px',
-  paddingBottom: '15px',
+  padding: `${theme.spacing(2)} ${theme.spacing(3)}`,
-  backgroundImage: 'linear-gradient(to top, hsla(0, 0%, 90%, .05), #e6e6e6)',
+  backgroundImage: `linear-gradient(to top, ${theme.palette.grey[200]}0D, ${theme.palette.grey[200]})`,
}));

These changes will help maintain consistency with the Material-UI theming system and improve the overall maintainability of the component.

📝 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
export const StyledDialogActions = styled(DialogActions)(() => ({
paddingTop: '15px',
paddingRight: '25px',
paddingBottom: '15px',
backgroundImage: 'linear-gradient(to top, hsla(0, 0%, 90%, .05), #e6e6e6)',
}));
export const StyledDialogActions = styled(DialogActions)(({ theme }) => ({
padding: `${theme.spacing(2)} ${theme.spacing(3)}`,
backgroundImage: `linear-gradient(to top, ${theme.palette.grey[200]}0D, ${theme.palette.grey[200]})`,
}));

8 changes: 8 additions & 0 deletions src/app/components/StyledDialogContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { styled } from '@mui/material';

export const StyledDialogContent = styled('div')(() => ({
gap: '15px',
margin: '15px',
display: 'flex',
flexDirection: 'column',
}));
5 changes: 5 additions & 0 deletions src/app/components/StyledDialogTitle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { DialogTitle, styled } from '@mui/material';

export const StyledDialogTitle = styled(DialogTitle)(() => ({
backgroundImage: 'linear-gradient(to bottom, hsla(0, 0%, 90%, .05), #e6e6e6)',
}));
21 changes: 3 additions & 18 deletions src/app/shared-links/Components/AddLinkDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ import {
import { FC, useEffect } from 'react';
import { useForm } from 'react-hook-form';

import { StyledDialogActions } from '@/app/components/StyledDialogActions';
import { StyledDialogContent } from '@/app/components/StyledDialogContent';
import { StyledDialogTitle } from '@/app/components/StyledDialogTitle';
import { useAuth } from '@/app/context/AuthProvider';
import { apiSharedLink } from '@/app/utils/api.class';
import { CreateSHLinkDto } from '@/domain/dtos/shlink';
Expand All @@ -19,24 +22,6 @@ const removeUndefinedValues = <T extends Record<string, unknown>>(
object: T,
): T => JSON.parse(JSON.stringify(object));

const StyledDialogTitle = styled(DialogTitle)(() => ({
backgroundImage: 'linear-gradient(to bottom, hsla(0, 0%, 90%, .05), #e6e6e6)',
}));

const StyledDialogContent = styled('div')(() => ({
gap: '15px',
margin: '15px',
display: 'flex',
flexDirection: 'column',
}));

const StyledDialogActions = styled(DialogActions)(() => ({
paddingTop: '15px',
paddingRight: '25px',
paddingBottom: '15px',
backgroundImage: 'linear-gradient(to top, hsla(0, 0%, 90%, .05), #e6e6e6)',
}));

export type TCreateSHLinkDto = Omit<CreateSHLinkDto, 'configExp'> & {
configExp?: string;
};
Expand Down
95 changes: 61 additions & 34 deletions src/app/shared-links/Components/LinksTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
'use client';

import { QrCode } from '@mui/icons-material';
import CalendarTodayIcon from '@mui/icons-material/CalendarToday';
import LinkOffIcon from '@mui/icons-material/LinkOff';
import {
Expand All @@ -14,21 +14,18 @@ import {
TablePagination,
TableRow,
Tooltip,
Dialog,
DialogActions,
DialogContent,
DialogContentText,
DialogTitle,
} from '@mui/material';
import { useEffect, useState } from 'react';
import React from 'react';

import { apiSharedLink } from '@/app/utils/api.class';
import { uuid } from '@/app/utils/helpers';
import { SHLinkMiniDto } from '@/domain/dtos/shlink';

import { AddLinkDialog } from './AddLinkDialog';
import BooleanIcon from './BooleanIcon';
import ConfirmationDialog from './ConfirmationDialog';
import { QRCodeDialog } from './QRCodeDialog';

interface Column {
id: keyof SHLinkMiniDto;
Expand All @@ -40,20 +37,26 @@ interface Column {
) => string | React.JSX.Element;
}

interface IActionColumn extends Omit<Column, 'id' | 'label'> {
interface IActionColumns extends Omit<Column, 'id' | 'label'> {
id: 'action';
label: React.JSX.Element;
action: (id: string) => void;
action: (row: SHLinkMiniDto) => void;
tooltipTitle?: string;
isDisabled?: (row: SHLinkMiniDto) => boolean;
}

const createActionColumn = (
label: React.JSX.Element,
action: (id: string) => void,
): IActionColumn => ({
action: (row: SHLinkMiniDto) => void,
tooltipTitle?: string,
isDisabled?: (row: SHLinkMiniDto) => boolean,
): IActionColumns => ({
id: 'action',
label,
minWidth: 50,
action,
tooltipTitle,
isDisabled,
});

const columns: readonly Column[] = [
Expand Down Expand Up @@ -90,12 +93,19 @@ export default function LinksTable() {
const [confirmDialogOpen, setConfirmDialogOpen] = useState(false);
const [selectedLinkId, setSelectedLinkId] = useState<string | null>(null);

const [qrCodeDialogOpen, setQrCodeDialogOpen] = useState(false);
const [qrCodeData, setQrCodeData] = useState<{
id: string;
managementToken: string;
url: string;
}>();

const handleChangePage = (_event: unknown, newPage: number) => {
setPage(newPage);
};

const handleDeactivate = async (id: string) => {
setSelectedLinkId(id);
const handleDeactivate = async (row: SHLinkMiniDto) => {
setSelectedLinkId(row.id);
setConfirmDialogOpen(true);
};

Expand All @@ -110,10 +120,19 @@ export default function LinksTable() {
}
}
};
const handleQrCode = async (row: SHLinkMiniDto) => {
setQrCodeDialogOpen(true);
setQrCodeData({ id: row.id, managementToken: '', url: row.url });
};

const actionColumn: IActionColumn[] = [
createActionColumn(<LinkOffIcon />, handleDeactivate),
// TODO: Other actions will be added
const actionColumns: IActionColumns[] = [
createActionColumn(
<LinkOffIcon />,
handleDeactivate,
'Deactivate',
(row) => row.active,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the isDisabled condition for the Deactivate button

The isDisabled function for the Deactivate action is currently (row) => row.active, which disables the button when the link is active. This is the opposite of the desired behavior. You likely want to disable the Deactivate button when the link is inactive.

Update the isDisabled function to reflect the correct logic:

-createActionColumn(
  ...
  'Deactivate',
-  (row) => row.active,
+  (row) => !row.active,
),
📝 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
(row) => row.active,
(row) => !row.active,

),
createActionColumn(<QrCode />, handleQrCode, 'Show QR Code'),
];

const fetchLinks = async () => {
Expand All @@ -136,8 +155,6 @@ export default function LinksTable() {
setAddDialog(true);
};

const combinedCols = [...columns, ...actionColumn];

return (
<Paper sx={{ width: '100%', overflow: 'hidden' }}>
<AddLinkDialog
Expand All @@ -150,6 +167,15 @@ export default function LinksTable() {
});
}}
/>
{qrCodeDialogOpen && (
<QRCodeDialog
open={qrCodeDialogOpen}
data={qrCodeData}
onClose={() => {
setQrCodeDialogOpen(false);
}}
/>
)}
Comment on lines +170 to +178
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure qrCodeData is defined before rendering QRCodeDialog

If qrCodeData is undefined, passing it to QRCodeDialog may cause runtime errors. To prevent this, ensure that qrCodeData is defined before rendering the dialog.

Modify the conditional rendering to include a check for qrCodeData:

-{qrCodeDialogOpen && (
+{qrCodeDialogOpen && qrCodeData && (
📝 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
{qrCodeDialogOpen && (
<QRCodeDialog
open={qrCodeDialogOpen}
data={qrCodeData}
onClose={() => {
setQrCodeDialogOpen(false);
}}
/>
)}
{qrCodeDialogOpen && qrCodeData && (
<QRCodeDialog
open={qrCodeDialogOpen}
data={qrCodeData}
onClose={() => {
setQrCodeDialogOpen(false);
}}
/>
)}

<Grid container justifyContent="end">
<Grid item>
<Button variant="contained" onClick={handleCreateLink}>
Expand Down Expand Up @@ -178,32 +204,33 @@ export default function LinksTable() {
.slice(page * rowsPerPage, page * rowsPerPage + rowsPerPage)
.map((row) => (
<TableRow hover tabIndex={-1} key={row.id}>
{combinedCols.map((column) => {
{columns.map((column) => {
const value = row[column.id];
return (
<TableCell
key={column.id + row.active}
align={column.align}
>
{column.id === 'action' ? (
<Tooltip title="Deactivate">
<span>
<Button
disabled={!row.active}
onClick={() => column.action(row.id)}
>
{column.label}
</Button>
</span>
</Tooltip>
) : column.format ? (
column.format(value)
) : (
value?.toString()
)}
{column.format
? column.format(value)
: value?.toString()}
</TableCell>
);
})}
<TableCell width={200}>
{actionColumns.map((actionColumn) => (
<Tooltip key={uuid()} title={actionColumn.tooltipTitle}>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using dynamic keys in lists; use stable keys instead

Using uuid() as the key prop in the actionColumns.map rendering generates a new unique key on every render. This can lead to performance issues and unexpected behavior because React relies on stable keys to track elements between renders.

Consider using a stable identifier for the key, such as the index or a unique property of actionColumn that remains constant.

Possible fix:

-{actionColumns.map((actionColumn) => (
-  <Tooltip key={uuid()} title={actionColumn.tooltipTitle}>
+{actionColumns.map((actionColumn, index) => (
+  <Tooltip key={index} title={actionColumn.tooltipTitle}>

Committable suggestion was skipped due to low confidence.

<span>
<Button
disabled={actionColumn.isDisabled?.(row)}
onClick={() => actionColumn.action(row)}
>
{actionColumn.label}
</Button>
</span>
</Tooltip>
))}
</TableCell>
</TableRow>
))}
</TableBody>
Expand Down
Loading
Loading