Adds readonly property to PillBadge and Modal button #5032
Conversation
WalkthroughThe changes in this pull request introduce new optional properties Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1)
9-9: Consider using object property shorthandThe renaming from
read_onlytoreadOnlyaligns with React naming conventions. However, you could simplify this using object property shorthand.- const { text, color, variant, showIcon, read_only: readOnly } = view; + const { text, color, variant, showIcon, readOnly } = view;app/packages/components/src/components/PillBadge/PillBadge.tsx (1)
Line range hint
8-21: Consider improving type safety with discriminated unions.The current props interface allows for three different types of
textprop with overlapping behavior. Consider using a discriminated union type to make the relationship betweentexttypes and their corresponding rendered components more explicit.Consider refactoring the props interface like this:
type BasePillBadgeProps = { color?: string; variant?: "filled" | "outlined"; showIcon?: boolean; operator?: () => void; readOnly?: boolean; }; type StaticPillBadgeProps = BasePillBadgeProps & { type: "static"; text: string; }; type SelectPillBadgeProps = BasePillBadgeProps & { type: "select"; text: string[]; }; type ColoredSelectPillBadgeProps = BasePillBadgeProps & { type: "coloredSelect"; text: [string, string][]; }; type PillBadgeProps = StaticPillBadgeProps | SelectPillBadgeProps | ColoredSelectPillBadgeProps;This would make the component's API more type-safe and self-documenting.
app/packages/components/src/components/ModalBase/ModalBase.tsx (3)
106-106: Consider using type-safe property accessWhile the disabled state is correctly propagated, consider using optional chaining for type-safe property access.
- disabled: props?.disabled, + disabled: props?.disabled ?? false,
Line range hint
174-186: Improve button state managementThere are several potential improvements for the button state management:
- The useEffect dependency array should include
primaryButtonView.disabledto prevent stale closures.- The functionality check could be simplified.
- The boolean operations for disabled state could be more explicit.
useEffect(() => { + const isTaggingMode = functionality.toLowerCase() === "tagging"; + const hasTags = primaryButtonView.params?.tags?.length > 0; + const isDisabledByProps = primaryButtonView.disabled ?? false; + if ( - (functionality === "tagging" || functionality === "Tagging") && - (!primaryButtonView.params || - !primaryButtonView.params.tags || - primaryButtonView.params.tags.length === 0) + isTaggingMode && !hasTags ) { setPrimaryButtonView({ ...primaryButtonView, - disabled: primaryButtonView?.disabled || true, + disabled: true, }); } else { setPrimaryButtonView({ ...primaryButtonView, - disabled: primaryButtonView?.disabled || false, + disabled: isDisabledByProps, }); } - }, [primaryButtonView.params]); + }, [primaryButtonView.params, primaryButtonView.disabled, functionality]);
Line range hint
1-270: Consider splitting button state logic into a custom hookThe button state management logic could be extracted into a custom hook (e.g.,
useModalButtonState) to improve reusability and maintainability. This would also make the component more focused on its primary responsibility of rendering the modal.Example structure:
const useModalButtonState = ( functionality: string, initialState: ButtonState, params: any ) => { // ... button state management logic return { buttonState, setButtonState }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/components/src/components/ModalBase/ModalBase.tsx(2 hunks)app/packages/components/src/components/PillBadge/PillBadge.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/components/src/components/ModalBase/ModalBase.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/components/src/components/PillBadge/PillBadge.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/components/PillBadgeView.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.
🔇 Additional comments (4)
app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (2)
4-4: LGTM: Type safety improvements
The addition of proper TypeScript types enhances type safety and improves code maintainability.
Also applies to: 6-6
14-14: Verify readOnly prop behavior
The readOnly prop is correctly passed to PillBadge, but let's verify its integration with the permissioning system.
✅ Verification successful
Let me verify the readOnly prop integration by checking its usage and permissions handling in the codebase.
Let me try a different search pattern to verify the readOnly prop implementation.
Let me try one more search to understand how the readOnly prop is used in the PillBadge component.
Let me try one final search to understand the PillBadge component implementation.
readOnly prop implementation is correct and properly integrated
The PillBadge component correctly implements the readOnly prop with proper TypeScript typing and default value handling:
- It's defined as an optional boolean prop with default value of false
- The prop is properly typed in the component interface
- The readOnly prop is passed through from SchemaIO component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent readOnly prop usage across components
# and its integration with permission checks
# Check for readOnly prop usage patterns
rg -A 5 'readOnly[?]?: ?boolean'
# Look for permission-related logic connected to readOnly
rg -A 10 'permission|access|role.*readOnly'
Length of output: 318216
Script:
#!/bin/bash
# Check PillBadge component implementation and readOnly prop usage
ast-grep --pattern $'const PillBadge = ({$$$, readOnly$_, $$$}) => {
$$$
}'
# Check for readOnly prop validation and handling
rg -A 10 "readOnly.*prop" app/packages/components/src/components/PillBadge/
# Check for any permission checks related to readOnly
rg -A 5 "readOnly.*permission|permission.*readOnly"
Length of output: 210
Script:
#!/bin/bash
# Search for PillBadge component definition and readOnly usage
rg -A 20 "PillBadge.*=.*{.*readOnly" app/packages/components/src/components/PillBadge/
# Check for readOnly prop type definition
rg -A 5 "readOnly\??: ?(boolean|PropTypes\.bool)" app/packages/components/src/components/PillBadge/
# Look for readOnly prop usage in SchemaIO components
rg -A 10 "readOnly.*PillBadge" app/packages/core/src/plugins/SchemaIO/
Length of output: 935
Script:
#!/bin/bash
# Search for PillBadge component implementation
rg -A 30 "const PillBadge = \({" app/packages/components/src/components/PillBadge/
# Check for readOnly usage in component logic
rg -A 10 "readOnly.*\?" app/packages/components/src/components/PillBadge/
# Look for PillBadge usage in SchemaIO components
rg -A 5 "<PillBadge.*readOnly" app/packages/core/src/plugins/SchemaIO/
Length of output: 1355
Script:
#!/bin/bash
# Search for PillBadge component definition with fixed pattern
rg -A 30 "const PillBadge" app/packages/components/src/components/PillBadge/
# Look for PillBadge usage in SchemaIO components with broader pattern
rg -A 5 "PillBadge.*readOnly|readOnly.*PillBadge" app/packages/core/src/plugins/SchemaIO/
# Check for readOnly prop handling in component logic
rg -A 10 "const.*readOnly|readOnly.*=|readOnly.*\?" app/packages/components/src/components/PillBadge/
Length of output: 4587
app/packages/components/src/components/PillBadge/PillBadge.tsx (1)
13-13: LGTM: Props interface changes look good.
The readOnly prop is properly typed and has a sensible default value.
Also applies to: 20-20
app/packages/components/src/components/ModalBase/ModalBase.tsx (1)
47-47: LGTM: Well-typed interface extension
The addition of the optional disabled property to ModalButtonView interface is properly typed and aligns with React's button props pattern.
| @@ -90,6 +92,7 @@ const PillBadge = ({ | |||
| ) : ( | |||
| <FormControl fullWidth> | |||
| <Chip | |||
| disabled={readOnly} | |||
There was a problem hiding this comment.
Consider extending readonly behavior.
While the disabled prop is correctly applied to the Chip component, the Select components within both conditional branches remain interactive even when readOnly is true.
Apply this diff to ensure consistent readonly behavior:
Array.isArray(text[0]) ? (
<Select
value={chipSelection}
variant={"standard"}
disableUnderline={true}
+ disabled={readOnly}
onChange={(event) => {
const selectedText = text.find(
(t) => t[0] === event.target.value
);
setChipSelection(event.target.value);
setChipColor(selectedText?.[1] ?? "default");
if (operator) {
handleClick(panelId, {
params: { value: event.target.value },
operator,
});
}
}}
sx={{
color: "inherit",
}}
>
{text.map((t, index) => (
<MenuItem key={index} value={t[0]}>
{t[0]}
</MenuItem>
))}
</Select>
) : (
<Select
value={chipSelection}
variant={"standard"}
disableUnderline={true}
+ disabled={readOnly}
onChange={(event) => {📝 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.
| disabled={readOnly} | |
| Array.isArray(text[0]) ? ( | |
| <Select | |
| value={chipSelection} | |
| variant={"standard"} | |
| disableUnderline={true} | |
| disabled={readOnly} | |
| onChange={(event) => { | |
| const selectedText = text.find( | |
| (t) => t[0] === event.target.value | |
| ); | |
| setChipSelection(event.target.value); | |
| setChipColor(selectedText?.[1] ?? "default"); | |
| if (operator) { | |
| handleClick(panelId, { | |
| params: { value: event.target.value }, | |
| operator, | |
| }); | |
| } | |
| }} | |
| sx={{ | |
| color: "inherit", | |
| }} | |
| > | |
| {text.map((t, index) => ( | |
| <MenuItem key={index} value={t[0]}> | |
| {t[0]} | |
| </MenuItem> | |
| ))} | |
| </Select> | |
| ) : ( | |
| <Select | |
| value={chipSelection} | |
| variant={"standard"} | |
| disableUnderline={true} | |
| disabled={readOnly} | |
| onChange={(event) => { |
There was a problem hiding this comment.
@manivoxel51, glad you agree. Let me know if there's anything else I can assist with.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @@ -10,12 +10,16 @@ const PillBadge = ({ | |||
| variant = "filled", | |||
| showIcon = true, | |||
| operator, | |||
| readOnly = false, | |||
There was a problem hiding this comment.
Why is this needed for PillBadge?
To set a readOnly PillBadge view you simply pass down a singular string. If only a string is given to this component (with or without a color) it becomes unselectable and is no longer a writeable component
There was a problem hiding this comment.
if we simply use
schema = {"text": "Disabled", "color": "secondary"}
badge = types.PillBadgeView(**schema)
The badge doesn't reflect it is a dropdown select - the downward caret will be missing. disabled / readOnly are legit properties to use in common components. if you really don't like the readOnly prop, happy to huddle and resolve!
There was a problem hiding this comment.
But isn't that the point of read only functionality? If I should only be reading the state of something, I don't understand why they would need to see the dropdown caret?
Having a dropdown caret implies that they can change the state of the pill, but we don't want that
So we should just show the pill and whatever the current status is without the ability to change it - which is just the string & a color choice
There was a problem hiding this comment.
ModalButtonView doesn't have a disabled field at the parent level so that makes sense
But I'm a bit confused as to why the changes to PillBadge & PillBadgeView are needed? If you just pass in a string, the component is automatically read-only
Here's an example:
schema = {"text": "Disabled", "color": "secondary"}
badge = types.PillBadgeView(**schema)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx (4)
Line range hint
11-13: Add TypeScript types for props and schemaThe component lacks proper TypeScript type definitions. Consider adding interfaces for the props and schema structure:
interface ImageViewProps { schema: { view?: { height?: number; width?: number; alt?: string; href?: string; operator?: string; prompt?: boolean; params?: any; cursor?: boolean; }; default?: string; }; data?: string; }
Line range hint
26-43: Optimize click handler with useCallbackThe
onClickhandler could be memoized to prevent unnecessary re-renders:- const onClick = () => { + const onClick = React.useCallback(() => { if (operator) { handleClick(panelId, { params: params, operator, prompt, callback: (result: OperatorResult) => { if (result?.error) { console.log(result?.error); console.log(result?.errorMessage); } else { openLink(); } }, }); } else { openLink(); } - }; + }, [operator, panelId, params, prompt, handleClick, openLink]);
19-19: Document the cursor property's relationship with read-only stateAdd a JSDoc comment to explain how the cursor property relates to the component's interactive state:
+ /** + * Whether to show a pointer cursor on hover. + * Defaults to false to support read-only states. + */ cursor = false,
Line range hint
33-36: Improve error handling feedbackCurrently, errors are only logged to the console. Consider adding user feedback:
if (result?.error) { - console.log(result?.error); - console.log(result?.errorMessage); + console.error('Operator execution failed:', result.error); + // Add user notification + props.onError?.(result.errorMessage || 'Operation failed'); } else {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/components/src/components/PillBadge/PillBadge.tsx(5 hunks)app/packages/core/src/plugins/SchemaIO/components/ImageView.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/components/src/components/PillBadge/PillBadge.tsx
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/ImageView.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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/core/src/plugins/SchemaIO/components/DropdownView.tsx (2)
160-161: Consider using span wrapper for disabled tooltipWhen a MUI Select is disabled, its tooltip might not be visible. Wrap the Tooltip with a span to ensure the tooltip remains accessible in disabled state.
- <Tooltip title={tooltipTitle}> - <Select + <Tooltip title={tooltipTitle}> + <span> + <SelectDon't forget to close the span tag after the Select component.
Also applies to: 163-163
202-210: Optimize onClick handler for disabled stateThe onClick handler with addOnClickToMenuItems check is unnecessary when the MenuItem is disabled. Consider moving this logic to a separate handler for better clarity.
- onClick={() => { - if (addOnClickToMenuItems) { - handleOnChange(value); - } - }} + onClick={addOnClickToMenuItems && !readOnly ? () => handleOnChange(value) : undefined}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/components/src/components/PillBadge/PillBadge.tsx(4 hunks)app/packages/core/src/plugins/SchemaIO/components/DropdownView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/components/src/components/PillBadge/PillBadge.tsx
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/DropdownView.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.
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/components/DropdownView.tsx (2)
1-1: LGTM: Clean import and prop addition
The Tooltip import and tooltipTitle prop are well-structured and follow React/TypeScript conventions.
Also applies to: 37-37
160-217: Verify accessibility and type consistency
Please ensure:
- The readOnly behavior is consistent with other dropdown components
- The tooltipTitle prop is included in the component's TypeScript interface
✅ Verification successful
Accessibility and type safety are properly implemented
The verification shows that:
- The
readOnlyprop is correctly implemented in the DropdownView component, being passed down to both the Select and MenuItem components consistently - The
tooltipTitleprop is properly defined in the component's view configuration and is correctly used with MUI's Tooltip component - The implementation follows the same pattern as other components in the codebase that use tooltips with selection elements
The code demonstrates good accessibility practices by:
- Using ARIA labels for icon buttons
- Implementing tooltips consistently
- Maintaining keyboard navigation support through the Select component
- Properly handling disabled states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar dropdown components and their readOnly implementation
ast-grep --pattern 'interface $interface {
$$$
readOnly?: boolean;
$$$
}'
# Search for tooltip implementations in other dropdown components
rg -l 'Tooltip.*Select' --type typescript
Length of output: 160
Script:
#!/bin/bash
# Search for similar dropdown components and their readOnly implementation
ast-grep --pattern 'interface $interface {
$$$
readOnly?: boolean;
$$$
}'
# Search for tooltip implementations in other dropdown components
rg -l 'Tooltip.*Select'
# Search for type definitions related to tooltipTitle
ast-grep --pattern 'interface $_ {
$$$
tooltipTitle?: $_
$$$
}'
# Search for other dropdown components with readOnly prop
rg 'readOnly.*Select' -A 2 -B 2
Length of output: 213
Script:
#!/bin/bash
# Search for Select components with props
ast-grep --pattern '<Select
$$$
readOnly={$_}
$$$
>'
# Find TypeScript interfaces containing Select
ast-grep --pattern 'interface $_Props {
$$$
}'
# Search for files containing Select components
fd Select --type f --extension tsx --extension ts
# Look for tooltip usage patterns
rg '<Tooltip' --type ts --type tsx -A 2
Length of output: 2716
Script:
#!/bin/bash
# Check the content of DropdownView.tsx and its interface definitions
cat app/packages/core/src/plugins/SchemaIO/components/DropdownView.tsx
# Check content of similar dropdown components
cat app/packages/core/src/components/GroupSliceSelector.tsx
cat app/packages/core/src/plugins/SchemaIO/components/FileExplorerView/VolumeSelector.tsx
# Look for tooltip patterns without file type specification
rg '<Tooltip.*title=' -A 1
Length of output: 11292
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/packages/components/src/components/ModalBase/ModalBase.tsx (3)
47-47: LGTM! Consider adding JSDoc documentation.The optional
disabledproperty is a good addition for permission control. Consider adding JSDoc documentation to explain its usage and relationship with permissions.+ /** Whether the button is disabled. Used for permission control in team environments. */ disabled?: boolean;
Line range hint
180-187: Simplify redundant disabled state logic.The current implementation has redundant logic as both conditions set the same disabled state. Additionally, the effect's dependency array might miss important dependencies.
- Simplify the redundant logic:
- if ( - (functionality === "tagging" || functionality === "Tagging") && - (!primaryButtonView.params || - !primaryButtonView.params.tags || - primaryButtonView.params.tags.length === 0) - ) { - setPrimaryButtonView({ - ...primaryButtonView, - disabled: primaryButtonView?.disabled || false, - }); - } else { - setPrimaryButtonView({ - ...primaryButtonView, - disabled: primaryButtonView?.disabled || false, - }); - } + setPrimaryButtonView({ + ...primaryButtonView, + disabled: primaryButtonView?.disabled || false, + });
- Update the dependency array to include all dependencies:
- }, [primaryButtonView.params]); + }, [primaryButtonView.params, primaryButtonView.disabled, functionality]);
Line range hint
8-56: Consider architectural improvements for better maintainability.
- Extract functionality types into an enum:
enum ModalFunctionality { TAGGING = 'tagging', NONE = 'none' }
- Consider splitting the component into smaller, focused components:
ModalHeaderModalButtonsTaggingSection
- Add proper TypeScript types for all props instead of using
any:interface ModalProps { props: { variant?: string; label?: string; disabled?: boolean; height?: string | number; width?: string | number; padding?: number; icon?: string; iconPosition?: 'left' | 'right'; } }Would you like me to help create these architectural improvements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/components/src/components/ModalBase/ModalBase.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/components/src/components/ModalBase/ModalBase.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.
Br2850
force-pushed
the
panels/add-readonly
branch
from
90082d7 to
b66bd9b
Compare
What changes are proposed in this pull request?
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?
fiftyonePython library changesSummary by CodeRabbit
New Features
disabledproperty to theModalButtonViewfor enhanced button control.readOnlyandtooltipTitleproperties to thePillBadgecomponent, allowing it to render in a non-interactive state and provide additional context.PillBadgeViewto reflect the newreadOnlyprop for better schema integration.tooltipTitleproperty to theDropdownViewfor enhanced user guidance.Improvements
PillBadgecomponent by restricting user input when in read-only mode.ImageViewcomponent to provide clearer user interaction feedback.