Add PersonTable #114

imths · 2024-11-22T22:48:40Z

Ticket Link

Groupings-1761

List of squashed commits

Add bogus functionality
Change linking in Admin page
Fix up issues from rebasing with main

Test Checklist

Unit Tests Passed
General Visual Inspection

JorWo · 2024-11-22T23:37:48Z

ui/src/app/admin/@tab/manage-person/page.tsx

+import PersonTable from '@/app/admin/_components/personTable';
+import { memberAttributeResults } from '@/lib/actions';
+
+const PersonTab = async (searchParams) => {


Make sure to set a type for searchParams, it would probably be: searchParams: { searchUid: string }. Also I recommend changing the searchParam from searchUid to uhIdentifier because that naming would cover both uid and uhUuid.

JorWo · 2024-11-22T23:40:45Z

ui/src/app/admin/@tab/manage-person/page.tsx

+import { memberAttributeResults } from '@/lib/actions';
+
+const PersonTab = async (searchParams) => {
+    const searchUid = searchParams.searchParams.searchUid;


Also I'm not sure why the searchParams object would make you access .searchParams

JorWo · 2024-11-22T23:43:59Z

ui/src/app/admin/@tab/manage-person/page.tsx

+    const groupingsInfo = await managePersonResults(searchUid);
+    const userInfo = searchUid === undefined ? undefined : (await memberAttributeResults([searchUid])).results[0];


Let's call these variables managePersonResults and memberAttributeResults so it's more clear what exactly we are passing into the PersonTable.

JorWo · 2024-11-22T23:44:58Z

ui/src/app/admin/@tab/manage-person/page.tsx

+    const props = { groupingsInfo: groupingsInfo, userInfo: userInfo, searchUid: searchUid };
+
+    return (
+        <>
+            <div className="container">
+                <PersonTable {...props} />


Instead of defining a props object, follow the other components and how they define each prop individually.

JorWo · 2024-11-22T23:46:03Z

Make sure all the file names are kebab case like person-table.tsx instead of camelCase like personTable.tsx.

JorWo · 2024-11-22T23:46:33Z

ui/src/app/admin/_components/personTable.tsx

+    const [rowSelection, setRowSelection] = useState<RowSelectionState>({});
+    const [modalOpen, setModalOpen] = useState(false);
+    const [modalData, setModalData] = useState([]);
+    const [dummyBool, setDummyBool] = useState(false);


What's this dummyBool for?

JorWo · 2024-11-22T23:55:17Z

ui/src/app/admin/_components/personTable.tsx

+        const numSelected = table.getSelectedRowModel().rows.length;
+        const arr = [];
+        let i;
+        for (i = 0; i < numSelected; i++) {
+            const original = table.getSelectedRowModel().rows[i].original;
+            if (original.inOwner) arr.push(original.path + ':owners');
+            if (original.inInclude) arr.push(original.path + ':include');
+            if (original.inExclude) arr.push(original.path + ':exclude');
+        }


There's a very nice way to rewrite this code using the .flatMap function: (Generated by ChatGPT!)

const groups = table.getSelectedRowModel().rows.flatMap(({ original }) => { const { path, inOwner, inInclude, inExclude } = original; return [ inOwner && `${path}:owners`, inInclude && `${path}:include`, inExclude && `${path}:exclude`, ].filter(groupPath => groupPath); });

JorWo · 2024-11-22T23:57:23Z

ui/src/app/admin/_components/personTable.tsx

+                    <p className="text-[1.2rem] font-medium text-text-color md:justify-end pt-5 ps-2">
+                        {userInfo === undefined
+                            ? ''
+                            : '(' + userInfo.name + ', ' + userInfo.uid + ', ' + userInfo.uhUuid + ')'}
+                    </p>


Instead of the ternary operator, it's nicer to use the && operator:

{userInfo !== undefined && <p className="text-[1.2rem] font-medium text-text-color md:justify-end pt-5 ps-2"> {'(' + userInfo.name + ', ' + userInfo.uid + ', ' + userInfo.uhUuid + ')'} </p>}

JorWo · 2024-11-22T23:57:46Z

ui/src/app/admin/_components/personTable.tsx

+            {validUid === 'FAILURE' && searchUid !== undefined ? (
+                <Alert variant="destructive" className="w-fit mb-7">
+                    <AlertDescription>{searchUid} is not in any grouping.</AlertDescription>
+                </Alert>
+            ) : (
+                ''
+            )}
+            {validUid === undefined && searchUid !== '' ? (
+                <Alert variant="destructive" className="w-fit mb-7">
+                    <AlertDescription>
+                        There was an error searching for {searchUid}. <br />
+                        Please ensure you have entered a valid UH member and try again.
+                    </AlertDescription>
+                </Alert>
+            ) : (
+                ''
+            )}
+            {searchUid === '' ? (
+                <Alert variant="destructive" className="w-fit mb-7">
+                    <AlertDescription>You must enter a UH member to search.</AlertDescription>
+                </Alert>
+            ) : (
+                ''
+            )}


Same goes for here, use the && operator instead of the ternary.

JorWo · 2024-11-22T23:59:05Z

ui/src/app/admin/_components/personTable.tsx

+                                    onClick={
+                                        header.column.id === 'name'
+                                            ? header.column.getToggleSortingHandler()
+                                            : () => void 0
+                                    }


You should be able to disable sorting for a certain column in your person-table-columns.tsx file. Then you can just use the getToggleSortingHandler() without this ternary,

JorWo · 2024-11-22T23:59:29Z

ui/src/app/admin/_components/personTable.tsx

+                {userInfo === undefined ? (
+                    ''
+                ) : (


Use the && operator instead of the ternary, thanks.

JorWo · 2024-11-23T00:01:26Z

ui/src/app/admin/_components/personTable.tsx

+                                                        <PersonTableTooltip
+                                                            value={'Manage grouping.'}
+                                                            side={'top'}
+                                                            desc={
+                                                                <Link
+                                                                    href={`/groupings/${cell.row.original.path}`}
+                                                                    rel="noopener noreferrer"
+                                                                    target="_blank"
+                                                                >
+                                                                    <ArrowUpRightFromSquare
+                                                                        size="1.25em"
+                                                                        className="text-text-primary"
+                                                                        data-testid={'arrow-up-right-from-square-icon'}
+                                                                    />
+                                                                </Link>
+                                                            }
+                                                        ></PersonTableTooltip>


It's preferred to pass in components as children instead of as a prop. Also the props value and side can be normal strings instead of strings wrapped {}.

JorWo · 2024-11-23T00:02:22Z

ui/src/app/admin/_components/personTable.tsx

+                                            {cell.column.id === 'inOwner' && (
+                                                <div className="ml-1">
+                                                    <p className={`${cell.row.original.inOwner ? 'text-red-500' : ''}`}>
+                                                        {cell.row.original.inOwner ? 'Yes' : 'No'}
+                                                    </p>
+                                                </div>
+                                            )}
+                                            {cell.column.id === 'inBasisAndInclude' && (
+                                                <div>
+                                                    <p
+                                                        className={`${
+                                                            cell.row.original.inBasisAndInclude ? 'text-red-500' : ''
+                                                        }`}
+                                                    >
+                                                        {cell.row.original.inBasisAndInclude ? 'Yes' : 'No'}
+                                                    </p>
+                                                </div>
+                                            )}
+                                            {cell.column.id === 'inInclude' && (
+                                                <div className="ml-2">
+                                                    <p
+                                                        className={`${
+                                                            cell.row.original.inInclude ? 'text-red-500' : ''
+                                                        }`}
+                                                    >
+                                                        {cell.row.original.inInclude ? 'Yes' : 'No'}
+                                                    </p>
+                                                </div>
+                                            )}
+                                            {cell.column.id === 'inExclude' && (
+                                                <div className="ml-2">
+                                                    <p
+                                                        className={`${
+                                                            cell.row.original.inExclude ? 'text-red-500' : ''
+                                                        }`}
+                                                    >
+                                                        {cell.row.original.inExclude ? 'Yes' : 'No'}
+                                                    </p>
+                                                </div>
+                                            )}
+                                            {cell.column.id === 'remove' && (
+                                                <div className="ml-3">
+                                                    <input
+                                                        type="checkbox"
+                                                        checked={row.getIsSelected()}
+                                                        onChange={row.getToggleSelectedHandler()}
+                                                    />
+                                                </div>
+                                            )}


These && statements should be moved to the person-table-columns.tsx file. Reference how the groupings-table-columns.tsx file does it.

JorWo · 2024-11-23T00:02:47Z

ui/src/app/admin/_components/personTable.tsx

+                    </TableBody>
+                )}
+            </Table>
+            {userInfo === undefined ? '' : <PaginationBar table={table} />}


Use the && operator here too.

JorWo · 2024-11-23T00:03:52Z

ui/src/app/admin/_components/personTableTooltip.tsx

+}: {
+    desc: React.ReactNode;
+    value: string;
+    side: Side | undefined;


I think the type for side is supposed to be a string or more precisely: "top" | "right" | "bottom" | "left"

JorWo · 2024-11-23T00:06:04Z

ui/src/components/ui/tooltip.tsx

+            `z-50 overflow-hidden rounded-md border border-slate-200 bg-white px-3 py-1.5
+       text-sm text-slate-950 shadow-md animate-in fade-in-0 zoom-in-95


Keep the tooltip as bg-black and text-white since most of the tooltips are those colors. If you are planning to use the tooltip with a white background and black text, change that component individually.

JorWo · 2024-11-23T00:08:00Z

ui/src/app/admin/_components/searchInput.tsx

+                        type="search"
+                        className="rounded-[-0.25rem] rounded-l-[0.25rem]"
+                        maxLength={8}
+                        placeholder={searchUid === null || searchUid === '' ? 'UH Username' : searchUid}


I don't think the placeholder needs to be the searchUid since the placeholder is just the text that displays when the input is empty.

JorWo · 2024-11-23T00:08:59Z

ui/src/components/modal/owners-modal.tsx

+    modalData
+}: {
+    open: boolean;
+    close: MouseEventHandler;


The type for close seems incorrect, maybe close should be renamed to onClose and the type would be: () => void

JorWo · 2024-11-23T00:14:26Z

ui/src/app/admin/_components/personTable.tsx

+                        <Button
+                            className="rounded-[-0.25rem] rounded-r-[0.25rem]"


I always thought it was weird that our button is not rounded on the left side. Let's keep the button normal and not straighten the left side, thanks.

JorWo · 2024-11-23T00:15:34Z

Could you switch out these Lucide React icons for FontAwesome? Thanks.

JorWo · 2024-11-23T00:17:30Z

There should be a remove modal that appears after pressing the remove button:

JorWo · 2024-11-23T00:18:08Z

Missing the "No groupings have been selected" alert:

JorWo · 2024-11-23T00:19:25Z

I think we can go without the tooltip arrows, the shadcn/ui ones look a bit odd:

Also, for those tooltips the period at the end isn't needed. Additionally, add the cursor-pointer class to the crown icon so the user knows it's clickable.

JorWo · 2024-11-23T00:20:44Z

I would love to see a manage-person-table-skeleton.tsx (similar to groupings-table-skeleton.tsx) as the loading file. The page seems unresponsive when fetching a lot of data, for example when searching mhodges.

Note that since this table doesn't use localStorage, you can just create a loading.tsx file next to page.tsx: https://nextjs.org/docs/app/building-your-application/routing/loading-ui-and-streaming

JorWo · 2024-11-23T00:25:15Z

This seems to be only an issue on Windows, the OwnersModal has scroll bars. Replacing overflow-scroll to overflow-auto class to AlertDialogDescription component should fix it

JorWo · 2024-11-23T00:29:27Z

Make sure the TailwindCSS handles smaller screen sizes, thanks.

JorWo · 2024-11-23T00:32:18Z

The Filter Groupings... box is missing the x button:

imths requested a review from JorWo November 22, 2024 22:48

imths force-pushed the dev-imths-1761 branch from 330f95b to 51ee57d Compare November 22, 2024 23:00

Add PersonTable

9e01de4

imths force-pushed the dev-imths-1761 branch from 51ee57d to 9e01de4 Compare November 22, 2024 23:10

JorWo reviewed Nov 22, 2024

View reviewed changes

JorWo reviewed Nov 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PersonTable #114

Add PersonTable #114

imths commented Nov 22, 2024

JorWo Nov 22, 2024 •

edited

Loading

JorWo Nov 22, 2024 •

edited

Loading

JorWo Nov 22, 2024

JorWo Nov 22, 2024

JorWo commented Nov 22, 2024

JorWo Nov 22, 2024

JorWo Nov 22, 2024

JorWo Nov 22, 2024

JorWo Nov 22, 2024

JorWo Nov 22, 2024

JorWo Nov 22, 2024

JorWo Nov 23, 2024

JorWo Nov 23, 2024

JorWo Nov 23, 2024

JorWo Nov 23, 2024

JorWo Nov 23, 2024

JorWo Nov 23, 2024

JorWo Nov 23, 2024

JorWo Nov 23, 2024

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024 •

edited

Loading

JorWo commented Nov 23, 2024 •

edited

Loading

JorWo commented Nov 23, 2024 •

edited

Loading

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024

		const groupingsInfo = await managePersonResults(searchUid);
		const userInfo = searchUid === undefined ? undefined : (await memberAttributeResults([searchUid])).results[0];

		`z-50 overflow-hidden rounded-md border border-slate-200 bg-white px-3 py-1.5
		text-sm text-slate-950 shadow-md animate-in fade-in-0 zoom-in-95

Add PersonTable #114

Are you sure you want to change the base?

Add PersonTable #114

Conversation

imths commented Nov 22, 2024

Ticket Link

List of squashed commits

Test Checklist

JorWo Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

JorWo Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorWo commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024 • edited Loading

JorWo commented Nov 23, 2024 • edited Loading

JorWo commented Nov 23, 2024 • edited Loading

JorWo commented Nov 23, 2024

JorWo commented Nov 23, 2024

JorWo Nov 22, 2024 •

edited

Loading

JorWo Nov 22, 2024 •

edited

Loading

JorWo commented Nov 23, 2024 •

edited

Loading

JorWo commented Nov 23, 2024 •

edited

Loading

JorWo commented Nov 23, 2024 •

edited

Loading