Skip to content

Commit

Permalink
fix: Enhance URL handling in table by rendering URL column types with…
Browse files Browse the repository at this point in the history
… `<a>` tag. (#37179)

## Description
<ins>Problem</ins>

URLs in table were not being rendered as links, resulting in
inconsistent user experience(missing context menus.

<ins>Root cause</ins>

URLs were rendered in `<div>` instead of `<a>`, making the component
lack links related features..

<ins>Solution</ins>

This PR handles... 

- Rendering URLs as links in BasicCell for a better user experience.
- Adding specific types for column properties for more robust data
validation and type checking.
- Adding unit tests for BasicCell functionality to ensure accurate
rendering and behavior.

- Simplifies the AutoToolTipComponent by removing unncessary
`LinkWrapper` component


Fixes #24769
_or_  
Fixes `Issue URL`
> [!WARNING]  
> _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.Table"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/11681339029>
> Commit: b7c5d17
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11681339029&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Table`
> Spec:
> <hr>Tue, 05 Nov 2024 10:23:38 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

- **New Features**
- Enhanced type safety for `compactMode` and `columnType` properties
across various components.
- Improved rendering logic in the `AutoToolTipComponent` for better
control based on `columnType`.
	- Optimized rendering in the `BasicCell` component using `useMemo`.

- **Bug Fixes**
- Resolved inconsistencies in type definitions for `BasicCell`,
`PlainTextCell`, and `SelectCell` components.
- Updated tooltip behavior in the `AutoToolTipComponent` to ensure
accurate rendering.

- **Tests**
- Introduced a new test suite for the `BasicCell` component, ensuring
proper rendering and interaction behaviors.
- Refined test cases for the `AutoToolTipComponent` to verify accurate
rendering under various conditions.
- Updated test case for URL column verification to check attributes
directly instead of navigation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
rahulbarwal authored Nov 6, 2024
1 parent 4e18827 commit 0288f5b
Show file tree
Hide file tree
Showing 8 changed files with 120 additions and 80 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,23 @@ describe(
table.ReadTableRowColumnData(3, 0, "v2").then(($cellData) => {
expect($cellData).to.eq("Profile pic");
});
table.AssertURLColumnNavigation(
0,
0,
"https://randomuser.me/api/portraits/med/women/39.jpg",
"v2",
);
table.AssertURLColumnNavigation(
3,
0,
"https://randomuser.me/api/portraits/med/men/52.jpg",
"v2",
);

agHelper
.GetElement(`${table._tableRowColumnData(0, 0, "v2")} a`)
.should(
"have.attr",
"href",
"https://randomuser.me/api/portraits/med/women/39.jpg",
)
.should("have.attr", "target", "_blank");
agHelper
.GetElement(`${table._tableRowColumnData(3, 0, "v2")} a`)
.should(
"have.attr",
"href",
"https://randomuser.me/api/portraits/med/men/52.jpg",
)
.should("have.attr", "target", "_blank");
});
},
);
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ export enum IMAGE_VERTICAL_ALIGN {
}

export interface BaseCellComponentProps {
compactMode: string;
compactMode: CompactMode;
isHidden: boolean;
allowCellWrapping?: boolean;
horizontalAlignment?: CellAlignment;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ interface Props {
children: React.ReactNode;
title: string;
tableWidth?: number;
columnType?: string;
columnType?: ColumnTypes;
className?: string;
compactMode?: string;
allowCellWrapping?: boolean;
Expand All @@ -152,51 +152,34 @@ interface Props {
isCellDisabled?: boolean;
}

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

return (
<CellWrapper
allowCellWrapping={props.allowCellWrapping}
cellBackground={props.cellBackground}
className="cell-wrapper"
compactMode={props.compactMode}
fontStyle={props.fontStyle}
horizontalAlignment={props.horizontalAlignment}
isCellDisabled={props.isCellDisabled}
isCellVisible={props.isCellVisible}
isHidden={props.isHidden}
isHyperLink
isTextType
onClick={(e: React.MouseEvent<HTMLDivElement>) => {
e.stopPropagation();
window.open(props.url, "_blank");
}}
textColor={props.textColor}
textSize={props.textSize}
verticalAlignment={props.verticalAlignment}
>
<div className="link-text">{content}</div>
<OpenNewTabIconWrapper className="hidden-icon">
<OpenNewTabIcon />
</OpenNewTabIconWrapper>
</CellWrapper>
);
}

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

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

switch (props.columnType) {
case ColumnTypes.BUTTON:
if (props.title) {
return content;
}

if (props.columnType === ColumnTypes.BUTTON && props.title) {
return content;
break;
case ColumnTypes.URL:
contentToRender = (
<>
<div className="link-text">{content}</div>
<OpenNewTabIconWrapper className="hidden-icon">
<OpenNewTabIcon />
</OpenNewTabIconWrapper>
</>
);
break;
default:
contentToRender = content;
}

return (
Expand All @@ -212,12 +195,13 @@ export function AutoToolTipComponent(props: Props) {
isCellDisabled={props.isCellDisabled}
isCellVisible={props.isCellVisible}
isHidden={props.isHidden}
isHyperLink={props.columnType === ColumnTypes.URL}
isTextType
textColor={props.textColor}
textSize={props.textSize}
verticalAlignment={props.verticalAlignment}
>
{content}
{contentToRender}
</CellWrapper>
</ColumnWrapper>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ test("does not show tooltip for non-button types", () => {

test("handles empty tooltip", () => {
const { getByText } = render(
<AutoToolTipComponent columnType={ColumnTypes.BUTTON} title="">
<AutoToolTipComponent columnType={ColumnTypes.BUTTON} title="Empty button">
<button>Empty button</button>
</AutoToolTipComponent>,
);
Expand Down Expand Up @@ -86,25 +86,6 @@ test("does not show tooltip for non-truncated text", () => {
expect(screen.queryByRole("dialog")).not.toBeInTheDocument();
});

test("opens a new tab for URL column type when clicked", () => {
const openSpy = jest.spyOn(window, "open").mockImplementation(() => null);

render(
<AutoToolTipComponent
columnType={ColumnTypes.URL}
title="Go to Google"
url="https://www.google.com"
>
<span>Go to Google</span>
</AutoToolTipComponent>,
);

fireEvent.click(screen.getByText("Go to Google"));
expect(openSpy).toHaveBeenCalledWith("https://www.google.com", "_blank");

openSpy.mockRestore();
});

describe("isButtonTextTruncated", () => {
function mockElementWidths(
offsetWidth: number,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React from "react";
import { render, screen, fireEvent } from "@testing-library/react";
import "@testing-library/jest-dom";
import { BasicCell, type PropType } from "./BasicCell";
import { ColumnTypes } from "widgets/TableWidgetV2/constants";
import { CompactModeTypes } from "widgets/TableWidget/component/Constants";

describe("BasicCell Component", () => {
const defaultProps: PropType = {
value: "Test Value",
onEdit: jest.fn(),
isCellEditable: false,
hasUnsavedChanges: false,
columnType: ColumnTypes.TEXT,
url: "",
compactMode: CompactModeTypes.DEFAULT,
isHidden: false,
isCellVisible: true,
accentColor: "",
tableWidth: 100,
disabledEditIcon: false,
disabledEditIconMessage: "",
};

it("renders the value", () => {
render(<BasicCell {...defaultProps} />);
expect(screen.getByText("Test Value")).toBeInTheDocument();
});

it("renders a link when columnType is URL", () => {
render(
<BasicCell
{...defaultProps}
columnType={ColumnTypes.URL}
url="http://example.com"
/>,
);
const link = screen.getByText("Test Value");

expect(link).toBeInTheDocument();
expect(link).toHaveAttribute("href", "http://example.com");
});

it("calls onEdit when double-clicked", () => {
render(<BasicCell {...defaultProps} isCellEditable />);
fireEvent.doubleClick(screen.getByText("Test Value"));
expect(defaultProps.onEdit).toHaveBeenCalled();
});

it("forwards ref to the div element", () => {
const ref = React.createRef<HTMLDivElement>();

render(<BasicCell {...defaultProps} ref={ref} />);
expect(ref.current).toBeInstanceOf(HTMLDivElement);
});
});
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import type { Ref } from "react";
import React, { useCallback } from "react";
import React, { useCallback, useMemo } from "react";
import { Tooltip } from "@blueprintjs/core";
import styled from "styled-components";
import type { BaseCellComponentProps } from "../Constants";
import type { BaseCellComponentProps, CompactMode } from "../Constants";
import { TABLE_SIZES } from "../Constants";
import { TooltipContentWrapper } from "../TableStyledWrappers";
import AutoToolTipComponent from "./AutoToolTipComponent";
import { importSvg } from "@appsmith/ads-old";
import { ColumnTypes } from "widgets/TableWidgetV2/constants";

const EditIcon = importSvg(
async () => import("assets/icons/control/edit-variant1.svg"),
Expand Down Expand Up @@ -55,7 +56,7 @@ const Content = styled.div`
const StyledEditIcon = styled.div<{
accentColor?: string;
backgroundColor?: string;
compactMode: string;
compactMode: CompactMode;
disabledEditIcon: boolean;
}>`
position: absolute;
Expand All @@ -74,12 +75,12 @@ const StyledEditIcon = styled.div<{
}
`;

type PropType = BaseCellComponentProps & {
export type PropType = BaseCellComponentProps & {
accentColor: string;
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
value: any;
columnType: string;
columnType: ColumnTypes;
tableWidth: number;
isCellEditable?: boolean;
isCellEditMode?: boolean;
Expand Down Expand Up @@ -128,6 +129,18 @@ export const BasicCell = React.forwardRef(
},
[onEdit, disabledEditIcon, isCellEditable],
);
const contentToRender = useMemo(() => {
switch (columnType) {
case ColumnTypes.URL:
return (
<a href={url} rel="noopener noreferrer" target="_blank">
{value}
</a>
);
default:
return value;
}
}, [columnType, url, value]);

return (
<Wrapper
Expand Down Expand Up @@ -157,7 +170,7 @@ export const BasicCell = React.forwardRef(
url={url}
verticalAlignment={verticalAlignment}
>
<Content ref={contentRef}>{value}</Content>
<Content ref={contentRef}>{contentToRender}</Content>
</StyledAutoToolTipComponent>
{isCellEditable && (
<StyledEditIcon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export type RenderDefaultPropsType = BaseCellComponentProps & {
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
value: any;
columnType: string;
columnType: ColumnTypes;
tableWidth: number;
isCellEditable: boolean;
isCellEditMode?: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from "../Constants";
import { CellWrapper } from "../TableStyledWrappers";
import { BasicCell } from "./BasicCell";
import type { ColumnTypes } from "widgets/TableWidget/component/Constants";

const StyledSelectComponent = styled(SelectComponent)<{
accentColor: string;
Expand Down Expand Up @@ -61,7 +62,7 @@ type SelectProps = BaseCellComponentProps & {
alias: string;
accentColor: string;
autoOpen: boolean;
columnType: string;
columnType: ColumnTypes;
borderRadius: string;
options?: DropdownOption[];
onFilterChange: (
Expand Down

0 comments on commit 0288f5b

Please sign in to comment.