-
Notifications
You must be signed in to change notification settings - Fork 557
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.
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
Adds disabled state for all button view variants #4930
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/plugins/SchemaIO/utils/style.ts (1)
11-20
: Approve with suggestions for improvementThe new
getDisabledColors
function is well-implemented and consistent with the existing color scheme. However, I have a few suggestions to enhance its robustness and readability:
Consider using more descriptive variable names. For example,
primaryColors
instead of the first element in the returned array, andsecondaryColors
for the second element.Add an explicit return type for better type safety. For example:
export function getDisabledColors(code: ColorType): [string, string] { // ... function body ... }Handle the case when
code
is undefined or null to prevent potential runtime errors. You could return a default value or throw a meaningful error.Here's a refactored version incorporating these suggestions:
export function getDisabledColors(code: ColorType): [string, string] { if (!code) { return ["var(--fo-palette-primary-main)", "var(--fo-palette-text-primary)"]; } const primaryColors = ["primary", "secondary", ...fiftyOneColorNames]; if (primaryColors.includes(code)) { return [ "var(--fo-palette-primary-main)", "var(--fo-palette-text-primary)", ]; } return [code, "var(--fo-palette-text-primary)"]; }app/packages/core/src/plugins/SchemaIO/components/ButtonView.tsx (1)
156-159
: Simplify the destructuring ofview
fromschema
In the
getColor
function, you're destructuringview
fromschema
again, which may be unnecessary sinceview
is already available in the parent scope. This can be simplified to improve code clarity.Apply this diff to use the already available
view
:function getColor(props: ViewPropsType) { - const { - schema: { view = {} }, - } = props; + const { schema } = props; + const { view = {} } = schema;Alternatively, pass
view
directly if it's frequently used.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/packages/core/src/plugins/SchemaIO/components/ButtonView.tsx (7 hunks)
- app/packages/core/src/plugins/SchemaIO/utils/style.ts (1 hunks)
- fiftyone/operators/types.py (0 hunks)
💤 Files with no reviewable changes (1)
- fiftyone/operators/types.py
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/core/src/plugins/SchemaIO/components/ButtonView.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/plugins/SchemaIO/utils/style.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
if (disabled) { | ||
const [bgColor, textColor] = getDisabledColors(color); | ||
baseProps.sx["&.Mui-disabled"] = { | ||
backgroundColor: variant === "outlined" ? "inherit" : bgColor, | ||
color: textColor, | ||
}; | ||
if (variant === "square") { | ||
baseProps.sx["&.Mui-disabled"].backgroundColor = (theme) => | ||
theme.palette.background.field; | ||
} | ||
|
||
if (variant === "outlined") { | ||
baseProps.sx["&.Mui-disabled"].backgroundColor = (theme) => | ||
theme.palette.background.field; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor disabled styles for consistency and maintainability
The disabled styles are being applied in both the getButtonProps
and getCommonProps
functions. This can lead to redundant code and potential style conflicts. Consolidating these styles into a single location would improve readability and maintainability.
Consider moving all disabled-related styling into getCommonProps
or getButtonProps
. For example, you can adjust getCommonProps
as follows:
function getCommonProps(props: ViewPropsType): ButtonProps {
const color = getColor(props);
const disabled = props.schema.view?.disabled || false;
return {
sx: {
color,
fontSize: "1rem",
fontWeight: "bold",
borderColor: color,
"&:hover": {
borderColor: color,
},
- ...(disabled
- ? {
- opacity: 0.5,
- }
- : {}),
},
+ ...(disabled && {
+ disabled: true,
+ }),
};
}
Then, handle the disabled-specific styles in getButtonProps
:
if (disabled) {
const [bgColor, textColor] = getDisabledColors(color);
baseProps.sx = {
...baseProps.sx,
backgroundColor: variant === "outlined" ? "inherit" : bgColor,
color: textColor,
opacity: 0.5,
};
if (variant === "square" || variant === "outlined") {
baseProps.sx.backgroundColor = (theme) => theme.palette.background.field;
}
}
This approach centralizes the disabled styling and avoids duplication.
Also applies to: 135-150
<TooltipProvider | ||
title={disabled ? "" : title} | ||
{...getComponentProps(props, "tooltip")} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide tooltip content even when the button is disabled
Currently, when the button is disabled, the tooltip title is set to an empty string, which prevents the tooltip from displaying. For better user experience and accessibility, it's advisable to display the tooltip regardless of the disabled state. This allows users to understand the purpose of the button and why it might be disabled.
Apply this diff to ensure the tooltip is always displayed:
<TooltipProvider
- title={disabled ? "" : title}
+ title={title}
{...getComponentProps(props, "tooltip")}
>
If necessary, you can provide a specific message explaining the disabled state.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
<TooltipProvider | |
title={disabled ? "" : title} | |
{...getComponentProps(props, "tooltip")} | |
> | |
<TooltipProvider | |
title={title} | |
{...getComponentProps(props, "tooltip")} | |
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏽
What changes are proposed in this pull request?
Adds a
disabled
property to the button view.all the variations enabled / disabled state
Screen.Recording.2024-10-15.at.6.20.53.PM.mov
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyone
Python library changesSummary by CodeRabbit
New Features
disabled
property for buttons, enhancing user experience by controlling button interactivity.IconButtonView
class for icon-based buttons, enriching the UI options.Bug Fixes
disabled
state.Documentation