Scale grid text #4747
Scale grid text #4747
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Grid
participant useFontSize
participant useSelect
User->>Grid: Interacts with Grid component
Grid->>useFontSize: Retrieve font size
useFontSize-->>Grid: Return font size
Grid->>useSelect: Update selection logic with font size
useSelect-->>Grid: Return updated selection
Grid-->>User: Render updated Grid with new font size
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
|
benjaminpkane
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
app/packages/core/src/components/Grid/useRefreshers.ts (1)
Line range hint
41-55: Consider adding error handling forrecords.current.clear.The
pageResetmemoization logic is correctly implemented, but error handling forrecords.current.clearcould be improved to ensure robustness.const pageReset = useMemo(() => { try { records.current.clear(); } catch (error) { console.error("Error clearing records:", error); } datasetName; extendedStagesUnsorted; filters; groupSlice; shouldRenderImaVidLooker; similarityParameters; view; return uuid(); }, [ datasetName, extendedStagesUnsorted, filters, groupSlice, records, shouldRenderImaVidLooker, similarityParameters, view, ]);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- app/packages/core/src/components/Grid/Grid.tsx (6 hunks)
- app/packages/core/src/components/Grid/useFontSize.ts (1 hunks)
- app/packages/core/src/components/Grid/useRefreshers.ts (3 hunks)
- app/packages/core/src/components/Grid/useSelect.ts (2 hunks)
- app/packages/core/src/components/Grid/useSelectSample.ts (2 hunks)
- app/packages/looker/src/elements/common/tags.module.css (2 hunks)
- app/packages/looker/src/elements/common/tags.ts (4 hunks)
- app/packages/looker/src/state.ts (1 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/looker/src/elements/common/tags.module.css
Additional context used
Path-based instructions (8)
app/packages/core/src/components/Grid/useFontSize.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.app/packages/core/src/components/Grid/useSelect.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.app/packages/core/src/components/Grid/useRefreshers.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.app/packages/core/src/components/Grid/useSelectSample.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.app/packages/core/src/components/Grid/Grid.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/state/src/hooks/useCreateLooker.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.app/packages/looker/src/state.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.app/packages/looker/src/elements/common/tags.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 not posted (22)
app/packages/core/src/components/Grid/useFontSize.ts (2)
1-2: LGTM!The import statements are correct and necessary.
4-6: LGTM!The constants are well-defined and necessary for the font size calculation.
app/packages/core/src/components/Grid/useSelect.ts (2)
1-1: LGTM!The import statements are correct and necessary.
8-8: LGTM!The function signature change is necessary for the dynamic font size adjustment.
app/packages/core/src/components/Grid/useRefreshers.ts (2)
7-7: LGTM!The import statement is correct and necessary.
9-9: LGTM!The function signature change is necessary for the page reset functionality.
app/packages/core/src/components/Grid/useSelectSample.ts (3)
1-1: Good practice: Type-only import.Changing the import of
IDto a type-only import clarifies its usage and can optimize the build process.
8-8: Good practice: Type-only import.Changing the import of
MutableRefObjectto a type-only import clarifies its usage and can optimize the build process.
18-18: Improvement: Exporting theRecordstype.Exporting the
Recordstype enhances its usability in other parts of the application, promoting a more modular design.app/packages/core/src/components/Grid/Grid.tsx (6)
21-21: Necessary import:useFontSize.The import of
useFontSizeis necessary for the new functionality introduced in theGridcomponent.
40-40: Verify the change:useRefresherswithrecords.Passing
recordstouseRefreshersmay alter its behavior in managing state resets. Ensure that this change is intentional and tested.
47-47: Necessary hook call:useFontSize.The
useFontSizehook is used to dynamically retrieve and utilize font size information based on theid. This is a necessary change for the new functionality.
83-89: Enhanced responsiveness:createLookerwithfontSize.Including the
fontSizeparameter in thecreateLookercall ensures that the font size is considered during the looker creation process. This enhances the component's responsiveness to font size adjustments.
99-99: Necessary dependency:getFontSize.Adding
getFontSizeto the dependencies array ensures that the memoized value is recalculated when the font size changes. This is necessary for the new functionality.
111-111: Necessary argument:useSelectwithgetFontSize.Including
getFontSizein theuseSelectcall ensures that the selection logic considers the current font size. This is necessary for the new functionality.app/packages/state/src/hooks/useCreateLooker.ts (2)
72-75: Enhanced flexibility:createfunction signature.The addition of the
extraparameter allows for more flexible handling of options when invoking thecreatefunction. This enhances the function's capability to accommodate additional data.
242-242: Seamless integration:extraparameter.Using the spread operator to merge the
extraoptions with existing options ensures that the new parameters integrate seamlessly into the existing logic. This is a good practice for maintaining code extensibility.app/packages/looker/src/state.ts (1)
157-157: LGTM!The addition of the
fontSizeproperty enhances the configurability of theBaseOptionsinterface without altering existing functionality.app/packages/looker/src/elements/common/tags.ts (4)
59-65: LGTM!Reordering class properties improves readability and maintainability.
64-64: LGTM!The addition of the
fontSizeproperty enhances the configurability of theTagsElementclass without altering existing functionality.
86-86: LGTM!Including the
fontSizeproperty in theoptionsobject ensures that the font size is configurable when creating the HTML element.
486-492: LGTM!The
handleFontmethod encapsulates the logic for updating the font size, improving code modularity and readability.
| export default (id: string) => { | ||
| const threshold = useThreshold(); | ||
|
|
||
| return useCallback(() => { | ||
| const width = document.getElementById(id)?.getBoundingClientRect().width; | ||
| if (!width) { | ||
| throw new Error("unexpected"); | ||
| } | ||
|
|
||
| return Math.max( | ||
| Math.min((width / threshold(width)) * SCALE_FACTOR, MAX), | ||
| MIN | ||
| ); | ||
| }, [id, threshold]); |
There was a problem hiding this comment.
Consider improving the error message.
The hook is correctly implemented, but the error message could be more descriptive to aid in debugging.
- throw new Error("unexpected");
+ throw new Error(`Element with id ${id} not found or has no width`);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 (id: string) => { | |
| const threshold = useThreshold(); | |
| return useCallback(() => { | |
| const width = document.getElementById(id)?.getBoundingClientRect().width; | |
| if (!width) { | |
| throw new Error("unexpected"); | |
| } | |
| return Math.max( | |
| Math.min((width / threshold(width)) * SCALE_FACTOR, MAX), | |
| MIN | |
| ); | |
| }, [id, threshold]); | |
| export default (id: string) => { | |
| const threshold = useThreshold(); | |
| return useCallback(() => { | |
| const width = document.getElementById(id)?.getBoundingClientRect().width; | |
| if (!width) { | |
| throw new Error(`Element with id ${id} not found or has no width`); | |
| } | |
| return Math.max( | |
| Math.min((width / threshold(width)) * SCALE_FACTOR, MAX), | |
| MIN | |
| ); | |
| }, [id, threshold]); |
| const fontSize = getFontSize(); | ||
| spotlight?.updateItems((id) => { | ||
| store.get(id)?.updateOptions({ | ||
| ...options, | ||
| fontSize, | ||
| selected: selected.has(id.description), | ||
| }); | ||
| }); | ||
| }); | ||
| }, [deferred, options, selected, spotlight, store]); | ||
| }, [deferred, getFontSize, options, selected, spotlight, store]); |
There was a problem hiding this comment.
Consider adding error handling for getFontSize.
The useEffect hook is correctly implemented, but error handling for getFontSize could be improved to ensure robustness.
useEffect(() => {
deferred(() => {
try {
const fontSize = getFontSize();
spotlight?.updateItems((id) => {
store.get(id)?.updateOptions({
...options,
fontSize,
selected: selected.has(id.description),
});
});
} catch (error) {
console.error("Error getting font size:", error);
}
});
}, [deferred, getFontSize, options, selected, spotlight, store]);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 fontSize = getFontSize(); | |
| spotlight?.updateItems((id) => { | |
| store.get(id)?.updateOptions({ | |
| ...options, | |
| fontSize, | |
| selected: selected.has(id.description), | |
| }); | |
| }); | |
| }); | |
| }, [deferred, options, selected, spotlight, store]); | |
| }, [deferred, getFontSize, options, selected, spotlight, store]); | |
| useEffect(() => { | |
| deferred(() => { | |
| try { | |
| const fontSize = getFontSize(); | |
| spotlight?.updateItems((id) => { | |
| store.get(id)?.updateOptions({ | |
| ...options, | |
| fontSize, | |
| selected: selected.has(id.description), | |
| }); | |
| }); | |
| } catch (error) { | |
| console.error("Error getting font size:", error); | |
| } | |
| }); | |
| }, [deferred, getFontSize, options, selected, spotlight, store]); |
|
I recommend that we get Sejal's thoughts before shipping something like this |
ok... |
|
What happens when you have a lot of labels? |
Same as current behavior. Overflow Y scroll |
benjaminpkane
force-pushed
the
enhancement/scale-looker-text
branch
from
4694341 to
60c7c76
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/packages/core/src/components/Grid/useFontSize.ts (1)
1-22: LGTM with suggestionsThe custom
useFontSizehook is implemented correctly and follows best practices for React hooks. The use ofuseCallbackis appropriate to memoize the callback function.Consider the following suggestions to further improve the code:
Add JSDoc comments to document the purpose, parameters, and return value of the hook. This will improve the code's readability and maintainability.
Extract the magic numbers used in the font size calculation formula into named constants. This will make the code more readable and self-explanatory.
As mentioned in the past review comment, improve the error message to provide more context and aid in debugging.
- throw new Error("unexpected"); + throw new Error(`Element with id "${id}" not found or has no width`);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (9)
e2e-pw/src/oss/specs/grid-tagging.spec.ts-snapshots/grid-tagged-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/grid-tagging.spec.ts-snapshots/grid-tagged-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/grid-tagging.spec.ts-snapshots/grid-untagged-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/grid-tagging.spec.ts-snapshots/grid-untagged-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar-cifar.spec.ts-snapshots/hide-ship-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar-cifar.spec.ts-snapshots/hide-ship-invisible-cat-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar-cifar.spec.ts-snapshots/hide-ship-visible-cat-chromium-linux.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar-cifar.spec.ts-snapshots/not-visible-cat-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/sidebar/sidebar-cifar.spec.ts-snapshots/not-visible-cat-chromium-linux.pngis excluded by!**/*.png,!**/*.png
Files selected for processing (7)
- app/packages/core/src/components/Grid/Grid.tsx (5 hunks)
- app/packages/core/src/components/Grid/useFontSize.ts (1 hunks)
- app/packages/core/src/components/Grid/useSelect.ts (2 hunks)
- app/packages/looker/src/elements/common/tags.module.css (2 hunks)
- app/packages/looker/src/elements/common/tags.ts (4 hunks)
- app/packages/looker/src/state.ts (1 hunks)
- app/packages/state/src/hooks/useCreateLooker.ts (2 hunks)
Additional context used
Path-based instructions (6)
app/packages/core/src/components/Grid/useFontSize.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.app/packages/core/src/components/Grid/useSelect.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.app/packages/core/src/components/Grid/Grid.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/state/src/hooks/useCreateLooker.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.app/packages/looker/src/state.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.app/packages/looker/src/elements/common/tags.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 not posted (14)
app/packages/looker/src/elements/common/tags.module.css (2)
7-9: LGTM!The changes to the
display,gap, andjustify-contentproperties will improve the layout, spacing, and alignment of the tags, resulting in a cleaner and more readable presentation.
16-18: Looks good!The removal of the
paddingproperty and the explicit definition of thefont-sizeandline-heightproperties will contribute to a more compact and consistent appearance of the tags.app/packages/core/src/components/Grid/useSelect.ts (2)
8-8: LGTM!The addition of the
getFontSizeparameter is a valid change that allows the function to retrieve the font size dynamically. The type annotation() => numbercorrectly specifies thatgetFontSizeis a function that returns a number.
18-27: Address the past review comment.The changes correctly integrate the font size retrieved from
getFontSizeinto theupdateOptionsmethod call, and includinggetFontSizein the dependency array ensures that the effect re-runs whenever the font size changes.However, I wanted to remind you about the past review comment suggesting adding error handling for
getFontSize. It's still a valid concern that needs to be addressed.app/packages/core/src/components/Grid/Grid.tsx (3)
20-20: LGTM!The import statement for the
useFontSizehook is correctly added, following the proper syntax and naming convention. This hook will likely be used to enhance theGridcomponent's functionality by allowing it to retrieve and utilize font size settings.
51-51: Looks good!The
getFontSizeconstant is properly introduced, utilizing theuseFontSizehook with theidparameter to retrieve the font size. This retrieved value will likely be used within theGridcomponent to adjust the text size based on the zoom level, enhancing the component's responsiveness.
87-94: Great work!The changes made to incorporate the font size into the
Gridcomponent's initialization and selection logic are well-implemented. By passing thefontSizeoption obtained from thegetFontSizefunction to thecreateLooker.currentfunction call, the component can now adapt to font size changes. The addition ofgetFontSizeas a dependency to theuseMemohook ensures that the component re-renders when the font size changes. Furthermore, updating theuseSelecthook to acceptgetFontSizeas an argument allows the selection logic to consider the font size as well.These modifications enhance the component's responsiveness to font size adjustments, potentially improving the user interface's adaptability.
Also applies to: 103-103, 115-115
app/packages/state/src/hooks/useCreateLooker.ts (1)
72-75: LGTM!The changes to the
createfunction signature and the usage of theextraparameter are well-implemented and follow best practices. The addition of theextraparameter improves the flexibility and extensibility of theuseCreateLookerhook by allowing dynamic configuration of Looker instances.The default value of an empty object for the
extraparameter ensures backward compatibility and prevents any breaking changes to the existing functionality.The use of object spread syntax to merge the
extraoptions with the existingoptionsobject is a clean and concise way to handle the additional configuration.Overall, the changes enhance the usability and maintainability of the code without introducing any issues.
Also applies to: 242-242
app/packages/looker/src/state.ts (1)
157-157: LGTM!The addition of the optional
fontSizeproperty to theBaseOptionsinterface is a good change that aligns with the PR objective. It provides the necessary flexibility to consumers of the interface.app/packages/looker/src/elements/common/tags.ts (5)
57-58: LGTM!The constants are well-named and their values follow best practices for maintaining consistent vertical rhythm in the layout.
62-62: LGTM!The reordering of the properties enhances clarity and maintainability. The new
attributeVisibilityandfontSizeproperties seem to be related to controlling the visibility and font size of the tags, which could be useful features.Also applies to: 64-64, 66-67
99-99: LGTM!The encapsulation of font-related styling logic in the
handleFontmethod improves the readability of therenderSelfmethod and promotes a more modular design.
488-503: LGTM!The
handleFontmethod is well-implemented and follows best practices for maintaining consistent vertical rhythm in the layout. The method efficiently updates the font size, line height, and spacing of the tags element based on the providedfontSize.
86-86: Verify the usage of the new parameters.The addition of
attributeVisibility,fontSize, andtimeZoneparameters to therenderSelfmethod suggests that they are now essential for rendering the tags element. Please ensure that these parameters are being used correctly and consistently throughout the codebase.Run the following script to verify the usage of the new parameters:
Also applies to: 89-90, 93-93
Verification successful
Parameters are used consistently and correctly.
The parameters
attributeVisibility,fontSize, andtimeZoneare integrated into the codebase and used in various contexts consistent with their intended purposes. No issues were found regarding their usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new parameters in the `renderSelf` method. # Test: Search for the usage of `attributeVisibility`. Expect: Consistent usage throughout the codebase. rg --type typescript -A 5 $'attributeVisibility' # Test: Search for the usage of `fontSize`. Expect: Consistent usage throughout the codebase. rg --type typescript -A 5 $'fontSize' # Test: Search for the usage of `timeZone`. Expect: Consistent usage throughout the codebase. rg --type typescript -A 5 $'timeZone'Length of output: 226
Script:
#!/bin/bash # Description: Verify the usage of the new parameters in the `renderSelf` method. # Find all TypeScript files and search for the usage of `attributeVisibility`, `fontSize`, and `timeZone`. fd -e ts -e tsx -x rg 'attributeVisibility' {} fd -e ts -e tsx -x rg 'fontSize' {} fd -e ts -e tsx -x rg 'timeZone' {}Length of output: 8406
What changes are proposed in this pull request?
Per a request from @danielgural and community members, here is a proposal to scale grid text with respect to the zoom setting (ideal row height)
Screen.Recording.2024-08-29.at.9.05.47.AM.mov
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Style