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: add select for sorting flow list #4137

Merged
merged 4 commits into from
Jan 17, 2025
Merged

feat: add select for sorting flow list #4137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d896fc
initial work on ordering
RODO94 Jan 8, 2025
7081351
add search params to sort
RODO94 Jan 10, 2025
8c48a5b
add changes and type-fest to editor
RODO94 Jan 15, 2025
90e8b19
add help description and new sort for direciton
RODO94 Jan 17, 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
3 changes: 2 additions & 1 deletion editor.planx.uk/src/pages/FlowEditor/lib/store/editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
ComponentType as TYPES,
flatFlags,
FlowGraph,
FlowStatus,
NodeId,
OrderedFlow,
} from "@opensystemslab/planx-core/types";
Expand Down Expand Up @@ -149,7 +150,7 @@ export interface FlowSummary {
id: string;
name: string;
slug: string;
status: "online" | "offline";
status: FlowStatus;
updatedAt: string;
operations: FlowSummaryOperations[];
publishedFlows: PublishedFlowSummary[];
Expand Down
23 changes: 14 additions & 9 deletions editor.planx.uk/src/pages/Team.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@ import { Link, useNavigation } from "react-navi";
import { FONT_WEIGHT_SEMI_BOLD } from "theme";
import { borderedFocusStyle } from "theme";
import { AddButton } from "ui/editor/AddButton";
import {
SortableFields,
SortControl,
} from "ui/editor/SortControl";
import { SortableFields, SortControl } from "ui/editor/SortControl";
import { slugify } from "utils";

import { client } from "../lib/graphql";
Expand Down Expand Up @@ -311,15 +308,23 @@ const Team: React.FC = () => {
);
const [flows, setFlows] = useState<FlowSummary[] | null>(null);

const sortArray: SortableFields<FlowSummary>[] = [
{ displayName: "Name", fieldName: "name" },
{ displayName: "Last updated", fieldName: "updatedAt" },
const sortOptions: SortableFields<FlowSummary>[] = [
{
displayName: "Name",
fieldName: "name",
directionNames: { asc: "A - Z", desc: "Z - A" },
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! When we have more use cases for this we may find we want these display values centralised within the component, and just allow consumers to define by type (e.g. direction: "alphabetical" | "date"). This is perfect for now 👍

},
{
displayName: "Last updated",
fieldName: "updatedAt",
directionNames: { asc: "Oldest first", desc: "Newest first" },
},
{
displayName: "Last published",
fieldName: `publishedFlows.0.publishedAt`,
directionNames: { asc: "Oldest first", desc: "Newest first" },
},
];

const fetchFlows = useCallback(() => {
getFlows(teamId).then((flows) => {
// Copy the array and sort by most recently edited desc using last associated operation.createdAt, not flow.updatedAt
Expand Down Expand Up @@ -368,7 +373,7 @@ const Team: React.FC = () => {
<SortControl<FlowSummary>
records={flows}
setRecords={setFlows}
sortOptions={sortArray}
sortOptions={sortOptions}
/>
)}
{teamHasFlows && (
Expand Down
91 changes: 49 additions & 42 deletions editor.planx.uk/src/ui/editor/SortControl.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import TrendingDownIcon from "@mui/icons-material/TrendingDown";
import TrendingUpIcon from "@mui/icons-material/TrendingUp";
import Box from "@mui/material/Box";
import IconButton from "@mui/material/IconButton";
import MenuItem from "@mui/material/MenuItem";
import { get } from "lodash";
import { get, orderBy } from "lodash";
import React, { useEffect, useMemo, useState } from "react";
import { useCurrentRoute, useNavigation } from "react-navi";
import { Paths } from "type-fest";
Expand All @@ -14,24 +11,28 @@ import SelectInput from "./SelectInput/SelectInput";
type SortDirection = "asc" | "desc";

export interface SortableFields<T> {
/** displayName is a string to use in the Select */
displayName: string;
/** fieldName is the key of the object used to sort */
fieldName: Paths<T>;
/** directionNames should be an object specifying display values for ascending and descending sort order */
directionNames: { asc: string; desc: string };
}

const compareValues = (
a: string | boolean,
b: string | boolean,
sortDirection: SortDirection,
) => {
if (a < b) {
return sortDirection === "asc" ? 1 : -1;
}
if (a > b) {
return sortDirection === "asc" ? -1 : 1;
}
return 0;
};

/**
* @component
* @description Sorts a list of objects
* @param {Type} T - a type to define the shape of the data in the records array
* @param {Array} props.records - an array of objects to sort
* @param {Function} props.setRecords - A way to set the new sorted order of the array
* @param {Array} props.sortOptions - An array of objects to define displayName, fieldName, and directionNames
* @returns {JSX.Element} Two select components to switch between fieldName and directionNames
* @example
* <SortControl<FlowSummary>
* records={flows}
* setRecords={setFlows}
* sortOptions={sortOptions}
* />
*/
export const SortControl = <T extends object>({
records,
setRecords,
Expand All @@ -51,12 +52,8 @@ export const SortControl = <T extends object>({
const selectedDisplaySlug = slugify(selectedSort.displayName);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably the part I feel most unsure about, but I'm comfortable that it delivers a unifying parameter I can use to search for sortOptions while adding it to the search params in a nice way for the user to see and read. My issue lay in the lack of readability in the key naming for deeply nested keys, and the fact displayName could really contain any sort of spaces and the like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's a good question - fieldName is guaranteed to be unique, but not necessarily readable.

There's no guarantee that slugify(selectedSort.displayName) doesn't return duplicates through right? It's unlikely but possible.

This might just be a data structure question. If we were to make sortOptions something like this our problems go away -

sortOptions: Record<string, SortableFields<T>>

...

  const sortArray: Record<string, SortableFields<FlowSummary>> = {
    name: { displayName: "Name", fieldName: "name" },
    lastUpdated: { displayName: "Last updated", fieldName: "updatedAt" },
    lastPublished: {
      displayName: "Last published",
      fieldName: `publishedFlows.0.publishedAt`,
    },
  }

This would -

  • Force a unique key
  • Provide us with a map already

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was an iteration I considered, I was thinking this is dealing with a complexity which is within the component (so not obvious when you add it to your table comp) so it is better to handle it within the component?

Imagining someone building this into their component and being confused why you have to add so many names, ultimately the same thing three times.

I can see the risk of duplicates but thought it was quite edge casey and resolvable.

But, I'm happy to integrate the change, I can see the benefits of the safety, so you don't need to try find your error in the naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is a fair point - lets aim to keep the complexity within the component I think 👍

Your version is clearer and simpler for consumers. The risk of duplicates is very very low!

const sortOptionsMap = useMemo(() => {
return sortOptions.reduce(
RODO94 marked this conversation as resolved.
Show resolved Hide resolved
(acc, option) => ({
...acc,
[slugify(option.displayName)]: option,
}),
{} as Record<string, SortableFields<T>>,
return Object.groupBy(sortOptions, ({ displayName }) =>
slugify(displayName),
);
}, [sortOptions]);

Expand All @@ -75,22 +72,25 @@ export const SortControl = <T extends object>({
);
};

useEffect(() => {
const parseStateFromURL = () => {
const { sort: sortParam, sortDirection: sortDirectionParam } =
route.url.query;
const matchingSortOption = sortOptionsMap[sortParam];
matchingSortOption && setSelectedSort(matchingSortOption);
if (!matchingSortOption) return;
setSelectedSort(matchingSortOption[0]);
if (sortDirectionParam === "asc" || sortDirectionParam === "desc") {
setSortDirection(sortDirection);
}
};

useEffect(() => {
parseStateFromURL();
}, []);

useEffect(() => {
const { fieldName } = selectedSort;
const sortedFlows = records?.sort((a: T, b: T) =>
compareValues(get(a, fieldName), get(b, fieldName), sortDirection),
);
sortedFlows && setRecords([...sortedFlows]);
const sortNewFlows = orderBy(records, fieldName, sortDirection);
setRecords(sortNewFlows);
updateSortParam(selectedDisplaySlug);
}, [selectedSort, sortDirection]);

Expand All @@ -101,7 +101,8 @@ export const SortControl = <T extends object>({
onChange={(e) => {
const targetKey = e.target.value as string;
const matchingSortOption = sortOptionsMap[targetKey];
matchingSortOption && setSelectedSort(matchingSortOption);
if (!matchingSortOption) return;
setSelectedSort(matchingSortOption?.[0]);
}}
>
{sortOptions.map(({ displayName }) => (
Expand All @@ -110,17 +111,23 @@ export const SortControl = <T extends object>({
</MenuItem>
))}
</SelectInput>
<IconButton
title="ordering"
aria-label="ordering"
onClick={() =>
sortDirection === "asc"
? setSortDirection("desc")
: setSortDirection("asc")
}
<SelectInput
value={sortDirection}
onChange={(e) => {
const newDirection = e.target.value as SortDirection;
setSortDirection(newDirection);
}}
>
{sortDirection === "asc" ? <TrendingUpIcon /> : <TrendingDownIcon />}
</IconButton>
<MenuItem key={slugify(selectedSort.directionNames.asc)} value={"asc"}>
{selectedSort.directionNames.asc}
</MenuItem>
<MenuItem
key={slugify(selectedSort.directionNames.desc)}
value={"desc"}
>
{selectedSort.directionNames.desc}
</MenuItem>
</SelectInput>
</Box>
);
};
Loading