Skip to content

fix:- added elipsis and tooltip to the button content in table widget component #36865

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.

Sign up for GitHub

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

Merged
yatinappsmith merged 8 commits into from
Oct 30, 2024
Merged

fix:- added elipsis and tooltip to the button content in table widget component #36865

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9b27dfc
fix:- added elipsis and tooltip to the button content in table widget…
skjameela Oct 14, 2024
48d92df
resolved review comments
skjameela Oct 21, 2024
92a26c9
resolved review comments for button tooltip
skjameela Oct 21, 2024
8ffde6e
resolved the prettier and linting issues
skjameela Oct 22, 2024
8f9d8d8
resolved review comments and added test cases
skjameela Oct 24, 2024
4798350
removed duplicate test cases
skjameela Oct 24, 2024
fa303f9
Resolved merge conflict in AutoTooltipComponent.test.tsx
skjameela Oct 24, 2024
36bfa53
resolved linting and prettier issues
skjameela Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/client/src/widgets/TableWidgetV2/component/TableStyledWrappers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ export const MenuColumnWrapper = styled.div<{ selected: boolean }>`

export const ActionWrapper = styled.div<{ disabled: boolean }>`
margin: 0 5px 0 0;
max-width: 100%;
${(props) => (props.disabled ? "cursor: not-allowed;" : null)}
&&&&&& {
.bp3-button {
Expand Down
42 changes: 35 additions & 7 deletions app/client/src/widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,40 @@ const MAX_WIDTH = 500;
const TOOLTIP_OPEN_DELAY = 500;
const MAX_CHARS_ALLOWED_IN_TOOLTIP = 200;

function useToolTip(children: React.ReactNode, title?: string) {
function isButtonTextTruncated(element: HTMLElement) {
const spanElement = element.querySelector("span");
const offsetWidth = spanElement?.offsetWidth ?? 0;
const scrollWidth = spanElement?.scrollWidth ?? 0;

return scrollWidth > offsetWidth;
Comment on lines +40 to +49

@rahulbarwal rahulbarwal Oct 23, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for null spanElement before accessing properties

If spanElement is null, comparison on line 44 will fail.

}

function useToolTip(
children: React.ReactNode,
title?: string,
isButton?: boolean,
) {
const ref = createRef<HTMLDivElement>();
const [requiresTooltip, setRequiresTooltip] = useState(false);

useEffect(() => {
let timeout: ReturnType<typeof setTimeout>;
const currentRef = ref.current;

const mouseEnterHandler = () => {
const element = ref.current?.querySelector("div") as HTMLDivElement;
if (!currentRef) return;

/*
* Using setTimeout to simulate hoverOpenDelay of the tooltip
* during initial render
*/
const mouseEnterHandler = () => {
timeout = setTimeout(() => {
const element = ref.current?.querySelector("div") as HTMLDivElement;

@rahulbarwal rahulbarwal Oct 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use currentRef variable here.

@rahulbarwal rahulbarwal Oct 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here: https://github.com/appsmithorg/appsmith/pull/36865/files#diff-8adcc3a82fc3414906b714b3cd1db7129035ae4a72e40de8b10c8afccc2b8823R77-R78

PS: We have some usages further down below as well, lets see if the variable can be reused.


/*
* Using setTimeout to simulate hoverOpenDelay of the tooltip
* during initial render
*/
if (element && element.offsetWidth < element.scrollWidth) {
setRequiresTooltip(true);
} else if (isButton && element && isButtonTextTruncated(element)) {
setRequiresTooltip(true);
} else {
setRequiresTooltip(false);
}
Expand All @@ -63,6 +80,7 @@ function useToolTip(children: React.ReactNode, title?: string) {
};

const mouseLeaveHandler = () => {
setRequiresTooltip(false);
clearTimeout(timeout);
};

Expand Down Expand Up @@ -158,13 +176,23 @@ function LinkWrapper(props: Props) {
);
}

function ButtonWrapper(props: Props) {
const content = useToolTip(props.children, props.title, true);

return content;
}

@coderabbitai coderabbitai Bot Oct 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider simplifying the ButtonWrapper component

Right now, ButtonWrapper simply returns the output of useToolTip with isButton set to true. If there's no extra logic needed for buttons, you might simplify your code by calling useToolTip directly in AutoToolTipComponent. This can make your code cleaner and easier to maintain.

@rahulbarwal rahulbarwal Oct 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment makes sense.

@coderabbitai coderabbitai Bot Oct 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rahulbarwal, excellent! I'm glad to hear that the suggestion makes sense. Simplifying the ButtonWrapper component will make the code cleaner and easier to maintain. If you have any questions or need further assistance, feel free to ask.

(^‿^)/

function AutoToolTipComponent(props: Props) {
const content = useToolTip(props.children, props.title);

if (props.columnType === ColumnTypes.URL && props.title) {
return <LinkWrapper {...props} />;
}

if (props.columnType === ColumnTypes.BUTTON && props.title) {
return <ButtonWrapper {...props} />;
}

return (
<ColumnWrapper className={props.className} textColor={props.textColor}>
<CellWrapper
Expand Down
43 changes: 27 additions & 16 deletions app/client/src/widgets/TableWidgetV2/component/cellComponents/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ import React, { useState } from "react";

import { ActionWrapper } from "../TableStyledWrappers";
import { BaseButton } from "widgets/ButtonWidget/component";
import type { ButtonColumnActions } from "widgets/TableWidgetV2/constants";
import {
ColumnTypes,
type ButtonColumnActions,
} from "widgets/TableWidgetV2/constants";
import styled from "styled-components";
import AutoToolTipComponent from "widgets/TableWidgetV2/component/cellComponents/AutoToolTipComponent";

const StyledButton = styled(BaseButton)<{
compactMode?: string;
Expand Down Expand Up @@ -44,21 +48,28 @@ export function Button(props: ButtonProps) {
e.stopPropagation();
}}
>
{props.isCellVisible && props.action.isVisible ? (
<StyledButton
borderRadius={props.action.borderRadius}
boxShadow={props.action.boxShadow}
buttonColor={props.action.backgroundColor}
buttonVariant={props.action.variant}
compactMode={props.compactMode}
disabled={props.isDisabled}
iconAlign={props.action.iconAlign}
iconName={props.action.iconName}
loading={loading}
onClick={handleClick}
text={props.action.label}
/>
) : null}
{props.isCellVisible && props.action.isVisible
? props.action.label && (

@rahulbarwal rahulbarwal Oct 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be rendered if label is null or undefined?
If null is expected to return then its better to append it to existing condition, which would look like:

{props.isCellVisible && props.action.isVisible &&  props.action.label 
    ? 
        <.../> 
        : null}

<AutoToolTipComponent
columnType={ColumnTypes.BUTTON}
title={props.action.label}
>
<StyledButton
borderRadius={props.action.borderRadius}
boxShadow={props.action.boxShadow}
buttonColor={props.action.backgroundColor}
buttonVariant={props.action.variant}
compactMode={props.compactMode}
disabled={props.isDisabled}
iconAlign={props.action.iconAlign}
iconName={props.action.iconName}
loading={loading}
onClick={handleClick}
text={props.action.label}
/>
</AutoToolTipComponent>
)
: null}
</ActionWrapper>
);
}