embeddings cta polish #5001
embeddings cta polish #5001
Conversation
WalkthroughThe changes involve significant modifications to the Changes
Possibly related PRs
Suggested reviewers
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.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
app/packages/embeddings/src/ComputeVisualizationButton.tsx (2)
Line range hint 23-36: Improve default button styling and accessibility.
Similar to the box variant, the default button needs accessibility improvements and consistent styling.
Apply these changes:
<Button
variant="contained"
+ aria-label="Compute visualization"
sx={{
backgroundColor: theme.primary.main,
color: theme.text.primary,
+ '&:hover': {
+ backgroundColor: theme.primary.dark,
+ },
}}
startIcon={<AddIcon />}
onClick={onClick}
>
- Compute Visualization
+ {BUTTON_TEXT}
</Button>Line range hint 1-36: Consider extracting button styles to a theme constant.
The button styles are duplicated and could be better managed through theme constants.
Consider creating a dedicated theme constant for the button styles:
// In your theme file
export const computeVisualizationButton = {
box: {
borderColor: theme.primary.main,
color: theme.primary.main,
'&:hover': {
borderColor: theme.primary.dark,
color: theme.primary.dark,
},
},
default: {
backgroundColor: theme.primary.main,
color: theme.text.primary,
'&:hover': {
backgroundColor: theme.primary.dark,
},
},
};
// Then in the component
sx={variant === "box" ? theme.computeVisualizationButton.box : theme.computeVisualizationButton.default}app/packages/embeddings/src/useComputeVisualization.ts (1)
17-39: Consider adding error tracking integration
While the hook effectively encapsulates the compute visualization logic, consider integrating with an error tracking system to monitor failed computations or invalid panel states in production.
This would help in:
- Monitoring computation failures
- Tracking invalid panel states
- Improving user experience by handling edge cases gracefully
app/packages/embeddings/src/EmptyEmbeddings.tsx (2)
6-12: Add TypeScript types for the compute visualization hook.
The computeViz object lacks explicit TypeScript types. Consider adding proper type definitions to improve type safety and code maintainability.
+interface ComputeVisualization {
+ isAvailable: boolean;
+ prompt: () => void;
+}
-const computeViz = useComputeVisualization();
+const computeViz: ComputeVisualization = useComputeVisualization();110-119: Add prop types and simplify the component structure.
The component could benefit from TypeScript prop types and a simpler structure.
+interface ComputeVisContentProps {
+ computeViz: {
+ prompt: () => void;
+ };
+}
-function ComputeVisContent({ computeViz }) {
+function ComputeVisContent({ computeViz }: ComputeVisContentProps) {
const theme = useTheme();
const secondaryBodyColor = theme.text.secondary;
return (
- <>
- <Grid item>
- <ComputeVisualizationButton onClick={() => computeViz.prompt()} />
- </Grid>
- </>
+ <Grid item>
+ <ComputeVisualizationButton onClick={() => computeViz.prompt()} />
+ </Grid>
);
}app/packages/embeddings/src/Embeddings.tsx (2)
Line range hint 1-30: Remove unused import ComputeVisualizationButton
Since the component has been refactored to use the new useComputeVisualization hook, the ComputeVisualizationButton import is no longer needed.
- import ComputeVisualizationButton from "./ComputeVisualizationButton";55-55: Consider adding explicit type annotation for computeViz
To improve type safety and code readability, consider adding an explicit type annotation for the computeViz object.
- const computeViz = useComputeVisualization();
+ const computeViz: ComputeVisualizationResult = useComputeVisualization();📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/packages/embeddings/src/ComputeVisualizationButton.tsx (2 hunks)
- app/packages/embeddings/src/Embeddings.tsx (4 hunks)
- app/packages/embeddings/src/EmptyEmbeddings.tsx (3 hunks)
- app/packages/embeddings/src/useComputeVisualization.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/embeddings/src/ComputeVisualizationButton.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/embeddings/src/Embeddings.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/embeddings/src/EmptyEmbeddings.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/embeddings/src/useComputeVisualization.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.
🔇 Additional comments (4)
app/packages/embeddings/src/useComputeVisualization.ts (1)
1-4: LGTM: Imports are well-organized and properly typed
The imports are clean, necessary, and properly scoped to their respective packages.
app/packages/embeddings/src/EmptyEmbeddings.tsx (1)
56-58: LGTM! Clean conditional rendering implementation.
The refactored code improves maintainability by centralizing the compute visualization logic in the hook and using clear conditional rendering.
app/packages/embeddings/src/Embeddings.tsx (2)
95-102: LGTM! Clean implementation of compute visualization option
The new compute visualization option is well-integrated into the existing UI pattern using PlotOption. The conditional rendering and event handling are implemented correctly.
Line range hint 1-102: Verify complete removal of old button styles
Let's ensure all old button styles have been cleaned up as mentioned in the PR objectives.
✅ Verification successful
Let me check the actual button style implementation to verify if there are any old styles remaining.
Button styles have been properly cleaned up
The button implementation now uses MUI's Button component with inline styles through the sx prop, and there are no old custom button styles remaining. The styling is consistent with the PR's cleanup objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining button-related styles in the codebase
rg -g '*.{ts,tsx,css,scss}' -i '(computevisualization.*button|button.*style)' app/packages/embeddings/
Length of output: 594
Script:
#!/bin/bash
# Check the implementation of ComputeVisualizationButton component
cat app/packages/embeddings/src/ComputeVisualizationButton.tsx
# Check for any styled-components related to buttons
rg -g '*.{ts,tsx}' 'styled.*button' app/packages/embeddings/src/
Length of output: 941
| <Button | ||
| variant="outlined" | ||
| sx={{ | ||
| borderColor: theme.primary.main, | ||
| color: theme.primary.main, | ||
| }} | ||
| startIcon={<AddIcon />} | ||
| onClick={onClick} | ||
| > | ||
| Compute Visualization | ||
| </Button> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance button accessibility and extract common styles.
The button lacks proper ARIA attributes and the styles could be better organized.
Consider these improvements:
<Button
variant="outlined"
+ aria-label="Compute visualization"
sx={{
borderColor: theme.primary.main,
color: theme.primary.main,
+ '&:hover': {
+ borderColor: theme.primary.dark,
+ color: theme.primary.dark,
+ },
}}
startIcon={<AddIcon />}
onClick={onClick}
>
- Compute Visualization
+ {BUTTON_TEXT}
</Button>And at the top of the file:
const BUTTON_TEXT = 'Compute Visualization';| import AddIcon from "@mui/icons-material/Add"; | ||
| import { useTheme } from "@fiftyone/components"; | ||
|
|
||
| export default function ComputeVisualizationButton({ uri }) { | ||
| const triggerEvent = usePanelEvent(); | ||
| export default function ComputeVisualizationButton({ variant, onClick }) { |
There was a problem hiding this comment.
Add TypeScript type definitions for props.
The component props should be properly typed for better type safety and documentation.
Apply this diff:
-export default function ComputeVisualizationButton({ variant, onClick }) {
+interface ComputeVisualizationButtonProps {
+ variant?: 'box';
+ onClick: () => void;
+}
+
+export default function ComputeVisualizationButton({
+ variant,
+ onClick
+}: ComputeVisualizationButtonProps) {📝 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.
| export default function ComputeVisualizationButton({ variant, onClick }) { | |
| interface ComputeVisualizationButtonProps { | |
| variant?: 'box'; | |
| onClick: () => void; | |
| } | |
| export default function ComputeVisualizationButton({ | |
| variant, | |
| onClick | |
| }: ComputeVisualizationButtonProps) { |
| const useFirstExistingUri = (uris: string[]) => { | ||
| const availableOperators = useMemo(() => listLocalAndRemoteOperators(), []); | ||
| return useMemo(() => { | ||
| const existingUri = uris.find((uri) => | ||
| availableOperators.allOperators.some((op) => op.uri === uri) | ||
| ); | ||
| const exists = Boolean(existingUri); | ||
| return { firstExistingUri: existingUri, exists }; | ||
| }, [availableOperators, uris]); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript types and documentation to improve maintainability
The hook logic is sound, but could benefit from better typing and documentation.
Consider applying these improvements:
+/**
+ * Hook to find the first existing URI from a list of URIs
+ * @param uris - Array of URIs to check
+ * @returns Object containing the first existing URI and whether it exists
+ */
-const useFirstExistingUri = (uris: string[]) => {
+interface UseFirstExistingUriResult {
+ firstExistingUri: string | undefined;
+ exists: boolean;
+}
+const useFirstExistingUri = (uris: string[]): UseFirstExistingUriResult => {
const availableOperators = useMemo(() => listLocalAndRemoteOperators(), []);
return useMemo(() => {
const existingUri = uris.find((uri) =>
availableOperators.allOperators.some((op) => op.uri === uri)
);
const exists = Boolean(existingUri);
return { firstExistingUri: existingUri, exists };
}, [availableOperators, uris]);
};📝 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.
| const useFirstExistingUri = (uris: string[]) => { | |
| const availableOperators = useMemo(() => listLocalAndRemoteOperators(), []); | |
| return useMemo(() => { | |
| const existingUri = uris.find((uri) => | |
| availableOperators.allOperators.some((op) => op.uri === uri) | |
| ); | |
| const exists = Boolean(existingUri); | |
| return { firstExistingUri: existingUri, exists }; | |
| }, [availableOperators, uris]); | |
| }; | |
| /** | |
| * Hook to find the first existing URI from a list of URIs | |
| * @param uris - Array of URIs to check | |
| * @returns Object containing the first existing URI and whether it exists | |
| */ | |
| interface UseFirstExistingUriResult { | |
| firstExistingUri: string | undefined; | |
| exists: boolean; | |
| } | |
| const useFirstExistingUri = (uris: string[]): UseFirstExistingUriResult => { | |
| const availableOperators = useMemo(() => listLocalAndRemoteOperators(), []); | |
| return useMemo(() => { | |
| const existingUri = uris.find((uri) => | |
| availableOperators.allOperators.some((op) => op.uri === uri) | |
| ); | |
| const exists = Boolean(existingUri); | |
| return { firstExistingUri: existingUri, exists }; | |
| }, [availableOperators, uris]); | |
| }; |
What changes are proposed in this pull request?
Refactors embeddings CTA logic into hook. Fixes issue where compute visualization button was displayed imporperly. Cleans up style of compute viz button in menu (see below).
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
ComputeVisualizationButtonwith improved styling and functionality based on thevariantprop.Embeddingscomponent that prompts users to compute a visualization.useComputeVisualization, to manage compute visualization operations.Bug Fixes
EmptyEmbeddingscomponent for better handling of compute visualization availability.Refactor