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

Jump to bottom

Tweak dropdownview to allow to display icon only #4881

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Tweak dropdownview to allow to display icon only #4881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
068f8fc
allow to only show icon in dropdownview
lanzhenw Oct 3, 2024
2e59859
update type and minor tweak
lanzhenw Oct 7, 2024
ea0394e
style tweak and clean up
lanzhenw Oct 7, 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
96 changes: 85 additions & 11 deletions app/packages/core/src/plugins/SchemaIO/components/DropdownView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { MenuItem, Select } from "@mui/material";
import React, { useState } from "react";
import { IconButton, MenuItem, Select } from "@mui/material";
import React, { useEffect, useMemo, useState } from "react";
import { useKey } from "../hooks";
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider moving iconImports to a separate configuration file

The addition of dynamic icon imports is a good practice for code splitting and performance optimization. However, to improve maintainability, consider moving the iconImports object to a separate configuration file. This would make it easier to add or modify supported icons in the future without changing the component logic.

You could create a new file, e.g., iconConfig.ts:

export const iconImports: {
  [key: string]: () => Promise<{ default: React.ComponentType<any> }>;
} = {
  MoreVertIcon: () => import("@mui/icons-material/MoreVert"),
  SettingsIcon: () => import("@mui/icons-material/Settings"),
};

Then import it in this file:

import { iconImports } from './iconConfig';

This approach would make the component more modular and easier to maintain.

Also applies to: 11-17

import { getComponentProps, getFieldSx } from "../utils";
import autoFocus from "../utils/auto-focus";
Expand All @@ -8,6 +8,14 @@ import AlertView from "./AlertView";
import ChoiceMenuItemBody from "./ChoiceMenuItemBody";
import FieldWrapper from "./FieldWrapper";

// if we want to support more icons in the future, add them here
const iconImports: {
[key: string]: () => Promise<{ default: React.ComponentType<any> }>;
} = {
MoreVertIcon: () => import("@mui/icons-material/MoreVert"),
SettingsIcon: () => import("@mui/icons-material/Settings"),
};

const MULTI_SELECT_TYPES = ["string", "array"];

export default function DropdownView(props: ViewPropsType) {
Expand All @@ -24,7 +32,10 @@ export default function DropdownView(props: ViewPropsType) {
description,
color,
variant,
icon,
} = view;
const [IconComponent, setIconComponent] =
useState<React.ComponentType<any> | null>(null);
const [key, setUserChanged] = useKey(path, schema, data, true);
const [selected, setSelected] = useState(false);

Expand Down Expand Up @@ -52,24 +63,84 @@ export default function DropdownView(props: ViewPropsType) {
? rawDefaultValue.toString().split(separator)
: rawDefaultValue;

const choiceLabels = choices.reduce((labels, choice) => {
labels[choice.value] = choice.label;
return labels;
}, {});
const choiceLabels = useMemo(() => {
return choices.reduce((labels, choice) => {
labels[choice.value] = choice.label;
return labels;
}, {});
}, [choices]);

const getIconOnlyStyles = () => ({
"&.MuiInputBase-root.MuiOutlinedInput-root.MuiInputBase-colorPrimary": {
backgroundColor: "transparent !important",
borderRadius: "0 !important",
border: "none !important",
boxShadow: "none !important",
"&:hover, &:focus": {
backgroundColor: "transparent !important",
boxShadow: "none !important",
},
},
"& .MuiSelect-select": {
padding: 0,
background: "transparent",
"&:focus": {
background: "transparent",
},
},
"& .MuiInputBase-root": {
background: "transparent",
},
"& .MuiOutlinedInput-notchedOutline": {
border: "none",
},
"& .MuiSelect-icon": {
display: "none",
},
});

const getDefaultStyles = (selected: boolean) => ({
".MuiSelect-select": {
padding: "0.45rem 2rem 0.45rem 1rem",
opacity: selected ? 1 : 0.5,
},
});

const getDropdownStyles = (icon: string | undefined, selected: boolean) => {
return icon ? getIconOnlyStyles() : getDefaultStyles(selected);
};

Comment on lines +73 to +111
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider further abstraction for styling functions

The addition of separate styling functions (getIconOnlyStyles, getDefaultStyles, and getDropdownStyles) improves code organization and maintainability. However, the styling objects are quite large and contain some repetition.

Consider the following improvements:

  1. Extract common styles into a base style object.
  2. Use a CSS-in-JS solution like styled-components or @emotion/styled for better style management.
  3. Create a separate styles file to keep the component logic and styling separate.

Example of extracting common styles:

const baseStyles = {
  backgroundColor: "transparent !important",
  boxShadow: "none !important",
};

const getIconOnlyStyles = () => ({
  "&.MuiInputBase-root.MuiOutlinedInput-root.MuiInputBase-colorPrimary": {
    ...baseStyles,
    borderRadius: "0 !important",
    border: "none !important",
    "&:hover, &:focus": baseStyles,
  },
  // ... rest of the styles
});

This approach would make the styling more maintainable and reduce duplication.

// Now, condense the code like this:
const { MenuProps = {}, ...selectProps } = getComponentProps(
props,
"select",
{
sx: {
".MuiSelect-select": {
padding: "0.45rem 2rem 0.45rem 1rem",
opacity: selected ? 1 : 0.5,
},
...getDropdownStyles(icon, selected),
...getFieldSx({ color, variant }),
},
}
);

// dynamically import the icon component
useEffect(() => {
if (icon && iconImports[icon]) {
iconImports[icon]().then((module) => {
setIconComponent(() => module.default);
});
}
}, [icon]);

const renderIcon = () => {
if (!IconComponent) return null;

return (
<IconButton aria-label={icon}>
<IconComponent />
</IconButton>
);
};

return (
<FieldWrapper {...props} hideHeader={compact}>
<Select
Expand All @@ -82,6 +153,9 @@ export default function DropdownView(props: ViewPropsType) {
displayEmpty
title={compact ? description : undefined}
renderValue={(value) => {
if (icon) {
return renderIcon();
}
const unselected = value?.length === 0;
setSelected(!unselected);
if (unselected) {
Expand Down Expand Up @@ -116,7 +190,7 @@ export default function DropdownView(props: ViewPropsType) {
>
{choices.map(({ value, ...choice }) => (
<MenuItem
key="value"
key={value}
value={value}
{...getComponentProps(props, "optionContainer")}
>
Expand Down
5 changes: 5 additions & 0 deletions fiftyone/operators/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,9 @@ def menu(self, name, **kwargs):
``"top-center"``, ``"top-right"``, ``"bottom-left"``, `"bottom-center"``, or
``"bottom-right"``. Overlay is useful when you want to display a floating menu on top of
another content (for example, menu for full-panel-width plot)
icon (None): when set, the icon will be displayed as the menu button instead of the label.
Can be "SettingsIcon", "MoreVertIcon".

Returns:
a :class:`Object`
"""
Expand Down Expand Up @@ -2361,6 +2364,8 @@ class MenuView(GridView):
``"top-center"``, ``"top-right"``, ``"bottom-left"``, `"bottom-center"``, or
``"bottom-right"``. Overlay is useful when you want to display a floating menu on top of
another content (for example, menu for full-panel-width plot)
icon (None): when set, the icon button will be displayed as the menu trigger,
instead of the selected value. Can be "SettingsIcon" or "MoreVertIcon"
Returns:
a :class:`Object`
"""
Expand Down
Loading