feat: add dropdown for layout thoroughness selection #620
Conversation
✅ Deploy Preview for velvety-vacherin-4193fb ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ameliorate-docs canceled.
|
There was a problem hiding this comment.
This is looking pretty good! The build failed for some linting errors (deploy log):
9:27:50 AM: 28:3 Error: Member 'MenuItem' of the import declaration should be sorted alphabetically. sort-imports
9:27:50 AM: 40:10 Error: 'NumberInput' is defined but never used. Allowed unused vars must match /^_/u. @typescript-eslint/no-unused-vars
See this comment #580 (comment) for what I think should help you get those errors to show in vscode. If you don't use vscode, I think you'll have to install prettier/eslint in whatever IDE you're using, or you can just run npm run build (which runs npm run lint - npm run dev will ignore linting issues) locally to make sure it builds without errors.
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Please keep the previous explanation here, it's still relevant
| <NumberInput | ||
| min={1} | ||
| max={100} | ||
| <Select |
There was a problem hiding this comment.
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:
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).
…ughness component
| 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"); | ||
|
|
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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
| // 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 |
There was a problem hiding this comment.
Please keep the previous explanation here, it's still relevant
| @@ -342,35 +343,37 @@ export const MoreActionsDrawer = ({ | |||
| </ToggleButton> | |||
| </ListItem> | |||
|
|
|||
| <ListItem disablePadding={false}> | |||
| <ListItem disablePadding> | |||
| <Typography variant="body2">Layout Thoroughness</Typography> | |||
There was a problem hiding this comment.
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.
| <Typography variant="body2">Layout Thoroughness</Typography> | |
| <Typography variant="body2">Thoroughness</Typography> |
| @@ -342,35 +343,37 @@ export const MoreActionsDrawer = ({ | |||
| </ToggleButton> | |||
| </ListItem> | |||
|
|
|||
| <ListItem disablePadding={false}> | |||
| <ListItem disablePadding> | |||
There was a problem hiding this comment.
By setting disablePadding to true, the row now goes all the way to the edge of the drawer, which is a bit awkward:
We should be keeping this false
| <ListItem disablePadding> | |
| <ListItem disablePadding={false}> |
| fullWidth // Use fullWidth for proper alignment | ||
| size="small" // Smaller size for better fit | ||
| sx={{ | ||
| marginLeft: "16px", // Adjust spacing |
There was a problem hiding this comment.
The dropdown is visually separated from the info icon already, so this marginLeft isn't necessary (Mui buttons give themselves padding by default)
Closes #585
Description of changes