fix: Update AutoToolTipComponent to use right position#34479
fix: Update AutoToolTipComponent to use right position#34479rahulbarwal wants to merge 5 commits intoreleasefrom
Conversation
WalkthroughThe changes across various files introduce a new Changes
Sequence Diagram(s)No sequence diagram required as changes primarily involve property additions and minor modifications. Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
Widgets Product
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9675155220. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (2)
Line range hint
32-32: Consider refactoring to use React hooks effectively.The
useToolTipfunction uses acreateRefwhich is not typically recommended within a hook. Consider usinguseRefinstead for better compatibility with functional component patterns.- const ref = createRef<HTMLDivElement>(); + const ref = useRef<HTMLDivElement>(null);
Line range hint
32-32: Potential memory leak inuseToolTip.The event listeners are removed only in the cleanup function of the
useEffect. If the component unmounts before the timeout triggers, it might not clean up properly. Consider clearing the timeout immediately after it's not needed.+ clearTimeout(timeout); ref.current?.removeEventListener("mouseenter", mouseEnterHandler); ref.current?.removeEventListener("mouseleave", mouseLeaveHandler);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (1 hunks)
Additional comments not posted (3)
app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (3)
Line range hint
122-122: Validate conditional rendering inAutoToolTipComponent.Ensure that all conditions (
columnTypeandtitle) are correctly validated before deciding the component rendering path. This is crucial for maintaining consistent behavior.Verification successful
Conditional rendering in
AutoToolTipComponentis correctly implemented.The conditions for rendering either
LinkWrapperorColumnWrapperwithCellWrapperbased oncolumnTypeandtitleare correctly validated.
- The component checks if
props.columnType === ColumnTypes.URLandprops.titleis truthy before renderingLinkWrapper.- If these conditions are not met, it renders
ColumnWrapperwithCellWrapper.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Validate the conditional rendering logic in `AutoToolTipComponent`. # Test: Check for all possible values of `columnType` and `title`. Expect: Correct component rendering for each case. rg --type typescript $'AutoToolTipComponent'Length of output: 78
Script:
#!/bin/bash # Correcting the file type and searching for the `AutoToolTipComponent` function definition. rg 'function AutoToolTipComponent' --type tsLength of output: 433
Script:
#!/bin/bash # Extracting the implementation of `AutoToolTipComponent` from the specified file. rg -A 50 'function AutoToolTipComponent' app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsxLength of output: 1151
Line range hint
99-99: Review the propagation handling inLinkWrapper.The
stopPropagationcall in theonClickhandler might interfere with other click handlers higher up in the component tree. Confirm that this behavior is intended.
Line range hint
32-32: Ensure tooltip visibility is handled correctly.The conditional rendering based on
requiresTooltipmight not re-render if the tooltip requirement changes post-initial render because the listeners are removed after the first check. Consider adding a dependency touseEffector re-attaching the listeners under certain conditions.
Since auto was creating problems when table was used in middle.
rahulbarwal
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx
rahulbarwal
changed the title
This commit adds the disableTooltip property to the TableWidgetV2 component. The disableTooltip property allows users to hide tooltips on cell hover. This feature was added in response to user feedback and will improve the user experience when working with tables.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9676722324. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (10)
app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx (1)
Line range hint
156-156: Redundant double-negation in title propertyThe static analysis tool flagged the use of redundant double-negation. This can lead to unnecessary complexity in code.
- title={!!value ? value.toString() : ""} + title={value ? value.toString() : ""}app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (2)
Line range hint
209-209: Replace unsafe isNaN with Number.isNaNThe use of
isNaNfor type coercion is unsafe and can lead to unexpected behavior. It's recommended to useNumber.isNaNfor a more reliable check.- if (isNaN(floatVal)) { + if (Number.isNaN(floatVal)) {Also applies to: 91-91
Line range hint
211-227: Remove unnecessary else clauseThe
elseclause in the currency formatting logic is unnecessary because the precedingifstatement already returns a value. Removing theelsecan simplify the control flow.- } else { + }app/client/src/widgets/TableWidgetV2/widget/index.tsx (7)
Line range hint
274-276: Redundant double-negation should be removed.The use of double-negation is unnecessary and can be simplified to improve code clarity.
- !!TableWidgetV2.getFeatureFlag(FEATURE_FLAG.rollout_js_enabled_one_click_binding_enabled) + TableWidgetV2.getFeatureFlag(FEATURE_FLAG.rollout_js_enabled_one_click_binding_enabled)
Line range hint
473-473: Use class name instead ofthisin static context.The use of
thisin the static methodgetFeatureFlagcan be confusing and should be replaced with the class name for clarity.- this.getFeatureFlag(ALLOW_TABLE_WIDGET_SERVER_SIDE_FILTERING) + TableWidgetV2.getFeatureFlag(ALLOW_TABLE_WIDGET_SERVER_SIDE_FILTERING)
Line range hint
650-650: Redundant double-negation should be removed.Similar to previous instances, remove the redundant double-negation to simplify the expression.
- !!Object.keys(this.props.primaryColumns).length + Object.keys(this.props.primaryColumns).length > 0
Line range hint
1935-1936: UseObject.hasOwninstead ofhasOwnProperty.To avoid potential issues with prototype chain modifications, use
Object.hasOwnfor property checks.- this.props.isEditableCellsValid.hasOwnProperty(columnsAlias) + Object.hasOwn(this.props.isEditableCellsValid, columnsAlias)
Line range hint
693-695: Remove unnecessaryelseclause.The
elseclause is not needed as the previous branches break early, simplifying the control flow.- else { + {
Line range hint
2147-2155: Wrap declarations in a block within switch cases.To prevent scope leakage, wrap the declarations inside the switch case blocks.
+ { let validationErrorMessage; if (isCellEditMode) { validationErrorMessage = column.validation.isColumnEditableCellRequired && (isNil(props.cell.value) || props.cell.value === "") ? "This field is required" : column.validation?.errorMessage; } + }
Line range hint
2877-2879: Remove unnecessaryelseclause.Similar to previous suggestions, removing the
elseclause can simplify the control flow.- else { + {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- app/client/src/widgets/TableWidgetV2/component/Constants.ts (4 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (7 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (2 hunks)
- app/client/src/widgets/TableWidgetV2/component/header/actions/Utilities.test.ts (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/index.tsx (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/General.ts (1 hunks)
- app/client/src/widgets/TableWidgetV2/widget/utilities.ts (2 hunks)
Files not reviewed due to errors (1)
- app/client/src/widgets/TableWidgetV2/widget/utilities.ts (no review received)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/TableWidgetV2/component/header/actions/Utilities.test.ts
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx
Additional context used
Biome
app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx
[error] 156-156: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negationapp/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx
[error] 211-227: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 91-91: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
[error] 209-209: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan)
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.app/client/src/widgets/TableWidgetV2/widget/utilities.ts
[error] 130-132: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 244-244: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 272-276: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 274-276: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 293-296: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 297-299: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 581-581: Duplicate case label. (lint/suspicious/noDuplicateCase)
The first similar label is here:
[error] 590-592: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 874-876: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1118-1120: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1121-1123: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1129-1133: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 1131-1133: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
app/client/src/widgets/TableWidgetV2/widget/index.tsx
[error] 274-276: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 473-473: Using this in a static context can be confusing. (lint/complexity/noThisInStatic)
this refers to the class.
Unsafe fix: Use the class name instead.
[error] 650-650: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 693-695: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 722-722: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 788-788: Avoid redundant double-negation. (lint/complexity/noExtraBooleanCast)
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
[error] 1935-1935: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 1936-1936: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 2147-2155: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 2178-2236: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 2190-2233: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 2207-2209: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 2480-2480: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause. (lint/correctness/noSwitchDeclarations)The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
[error] 2867-2867: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 2877-2879: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (8)
app/client/src/widgets/TableWidgetV2/component/cellComponents/BasicCell.tsx (2)
102-102: Addition of disableTooltip property to BasicCell PropsThe addition of
disableTooltipto theBasicCellprops is consistent with the changes described in the PR summary and AI-generated summary. This allows for better control over tooltip behavior, enhancing user experience by making tooltips less intrusive based on the configuration.
147-147: Proper propagation of disableTooltip propertyThe
disableTooltipproperty is correctly propagated toStyledAutoToolTipComponent. This ensures that the tooltip visibility can be dynamically adjusted, aligning with the changes intended in the PR.
[APROVED]app/client/src/widgets/TableWidgetV2/component/cellComponents/PlainTextCell.tsx (2)
121-121: Addition of disableTooltip property to PlainTextCell PropsJust like in
BasicCell.tsx, thedisableTooltipproperty has been added to the props ofPlainTextCell. This change is essential for allowing users to disable tooltips on cell hover, improving the usability of the table widget.
303-303: Proper propagation of disableTooltip propertyThe
disableTooltipproperty is correctly propagated to theBasicCellcomponent used withinPlainTextCell. This ensures that the tooltip visibility configuration is respected in all cell types.app/client/src/widgets/TableWidgetV2/widget/propertyConfig/PanelConfig/General.ts (1)
108-128: Configuration panel update to include disableTooltip propertyThe addition of the
disableTooltipproperty to the configuration panel allows users to easily toggle the visibility of tooltips from the UI. This is a user-friendly enhancement that aligns with the PR's objectives to improve user experience by providing more control over the tooltip behavior.app/client/src/widgets/TableWidgetV2/component/Constants.ts (2)
154-156: Introduction of DisableTooltipProperties interfaceThe new
DisableTooltipPropertiesinterface with adisableTooltipboolean property is a strategic addition, allowing for a consistent way to manage tooltip visibility across different components and properties within the TableWidgetV2.
228-228: Consistent integration of disableTooltip across interfacesThe
disableTooltipproperty has been consistently integrated across various interfaces (CellLayoutProperties,ColumnBaseProperties, andBaseCellComponentProps). This ensures that the new tooltip control feature is uniformly available throughout the TableWidgetV2 component.Also applies to: 292-292, 505-505
app/client/src/widgets/TableWidgetV2/widget/index.tsx (1)
2500-2500: Addition ofdisableTooltipproperty inPlainTextCellrendering.The
disableTooltipproperty has been correctly integrated into thePlainTextCellcomponent to control the visibility of tooltips based on the new property. This aligns with the PR's objective to enhance user experience by making tooltips less intrusive.
|
Deploy-Preview-URL: https://ce-34479.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9678216851. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx
|
Deploy-Preview-URL: https://ce-34479.dp.appsmith.com |
|
Closing this in favor of: #34507 |
1 participant
Description
This pull request updates the AutoToolTipComponent to use the "right" position instead of the fixed "bottom" position.
Fixes #23156
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Widget"
🔍 Cypress test results
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9675134106
Commit: 53f6626
Cypress dashboard.
Tags:
@tag.WidgetIt seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
disableTooltipproperty to various components and configuration panels, allowing users to hide tooltips on cell hover inTableWidgetV2.Bug Fixes
AutoToolTipComponent.Tests
disableTooltipproperty with default values.