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

Changes from 1 commit
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
28 changes: 17 additions & 11 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 @@ -24,6 +24,8 @@ import {
Divider,
Drawer,
IconButton,
Select,
MenuItem,
List,
ListItem,
ListItemIcon,
Expand Down Expand Up @@ -345,32 +347,36 @@ export const MoreActionsDrawer = ({
<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."
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} // allow touch to immediately trigger
leaveTouchDelay={Infinity} // touch-away to close on mobile, since message is long
>
<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) => {
const value = event.target.value;
setLayoutThoroughness(Number(value));
}}
keyserj marked this conversation as resolved.
Show resolved Hide resolved
/>
sx={{
marginLeft: "16px",
minWidth: "120px",
}}
keyserj marked this conversation as resolved.
Show resolved Hide resolved
>
<MenuItem value={1}>Low</MenuItem>
<MenuItem value={10}>Medium</MenuItem>
<MenuItem value={100}>High</MenuItem>
</Select>
</ListItem>

<Divider>Filter Config</Divider>
Expand Down