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 dropdown for layout thoroughness selection #620

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: add dropdown for layout thoroughness selection #620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b4fcf3
feat: add dropdown for layout thoroughness selection
shibv Dec 22, 2024
9bd1c49
fix: resolve linting errors and improve UI alignment for Layout Thoro…
shibv Dec 22, 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
2 changes: 1 addition & 1 deletion next-env.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
/// <reference types="next/image-types/global" />

// NOTE: This file should not be edited
// see https://nextjs.org/docs/pages/building-your-application/configuring/typescript for more information.
// see https://nextjs.org/docs/pages/api-reference/config/typescript for more information.
3 changes: 1 addition & 2 deletions src/web/common/components/Form/NumberInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ export const NumberInput = ({ name, min, max }: Props) => {
{...field}
min={min}
max={max}
value={field.value as number} // should be able to type-safe this but seems hard and not worth effort
value={field.value as number}
onChange={(event, value) => {
event.preventDefault(); // don't trigger default form submit, which would refresh the page
if (value === null) throw new Error("NumberInput should not allow empty value");

Comment on lines +27 to 30
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the changes in this file should be undone, unless you have particular reason for them.

The comment about type-safing still seems relevant to convey that the only reason it's not type-safe is because we didn't take the time for it.

The error is being thrown to be explicit that we've chosen not to handle a null value.

field.onChange(value);
submit();
Expand Down
33 changes: 18 additions & 15 deletions src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ import {
ListItem,
ListItemIcon,
ListItemText,
MenuItem,
Select,
ToggleButton,
Tooltip,
Typography,
} from "@mui/material";
import { toPng } from "html-to-image";
import { getRectOfNodes, getTransformForBounds } from "reactflow";

import { NumberInput } from "@/web/common/components/NumberInput/NumberInput";
import { getDisplayNodes } from "@/web/topic/components/Diagram/externalFlowStore";
import { downloadTopic, uploadTopic } from "@/web/topic/loadStores";
import { useOnPlayground } from "@/web/topic/store/topicHooks";
Expand Down Expand Up @@ -342,35 +343,37 @@ export const MoreActionsDrawer = ({
</ToggleButton>
</ListItem>

<ListItem disablePadding={false}>
<ListItem disablePadding>
Copy link
Collaborator

Choose a reason for hiding this comment

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

By setting disablePadding to true, the row now goes all the way to the edge of the drawer, which is a bit awkward:

image

We should be keeping this false

Suggested change
<ListItem disablePadding>
<ListItem disablePadding={false}>

<Typography variant="body2">Layout Thoroughness</Typography>
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned here https://github.com/amelioro/ameliorate/pull/620/files#r1894997107, since we're using Select still, we should remove Layout to keep this text from having to wrap to two lines.

Suggested change
<Typography variant="body2">Layout Thoroughness</Typography>
<Typography variant="body2">Thoroughness</Typography>

<Tooltip
title="Determines how much effort the layout algorithm puts into laying out nodes such that they efficiently use space. 1 = lowest effort, 100 = highest effort."
enterTouchDelay={0} // allow touch to immediately trigger
leaveTouchDelay={Infinity} // touch-away to close on mobile, since message is long
title="Determines how much effort the layout algorithm puts into laying out nodes such that they efficiently use space. Low = minimal effort, High = maximum effort."
enterTouchDelay={0} // Trigger immediately on touch
leaveTouchDelay={Infinity} // Close on touch-away
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep "since message is long" in the comment here - it explains why we're using touch-away to close, rather than the default timeout for Mui Tooltips

>
<IconButton
color="info"
aria-label="Thoroughness info"
sx={{
// Don't make it look like clicking will do something, since it won't.
// Using a button here is an attempt to make it accessible, since the tooltip will show
// on focus.
cursor: "default",
cursor: "default", // Maintain accessibility while preventing click actions
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this comment makes it seem like setting cursor: "default" is how we're maintaining accessibility. The previous comment explained that we're using a button for accessibility because it allows focus, and cursor: "default" is to ensure the button doesn't look like clicking will do something.

Side note: looking at this code again, it seems like we could just add a tabIndex to allow focus on the icon directly, instead of adding a button for that. I'm not sure why I didn't do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep the previous explanation here, it's still relevant

alignSelf: "center",
}}
>
<Info />
</IconButton>
</Tooltip>
<NumberInput
min={1}
max={100}
<Select
Copy link
Collaborator

@keyserj keyserj Dec 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Like with the watch dropdown, I would have used Mui's TextField with the select prop, because it uses a Mui Select component under-the-hood but comes with TextField's nice labeling, like this:

image

I think using Select here can work too, but then perhaps we could just use "Thoroughness" in the text label, so that the label doesn't go onto two lines ("Layout" I think is implied by being in the "Layout Config" section anyway).

value={layoutThoroughness}
onChange={(_event, value) => {
if (value) setLayoutThoroughness(value);
onChange={(event) => setLayoutThoroughness(Number(event.target.value))}
fullWidth // Use fullWidth for proper alignment
size="small" // Smaller size for better fit
sx={{
marginLeft: "16px", // Adjust spacing
Copy link
Collaborator

Choose a reason for hiding this comment

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

The dropdown is visually separated from the info icon already, so this marginLeft isn't necessary (Mui buttons give themselves padding by default)

}}
/>
>
<MenuItem value={1}>Low</MenuItem>
<MenuItem value={10}>Medium</MenuItem>
<MenuItem value={100}>High</MenuItem>
</Select>
</ListItem>

<Divider>Filter Config</Divider>
Expand Down