Skip to content
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.

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

fix(link): ensure Icon in component has inline display property to respect dimensions of the parent #6349

Merged
merged 2 commits into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 14 additions & 1 deletion cypress/components/link/link.cy.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
/* eslint-disable jest/valid-expect, jest/no-disabled-tests, no-unused-expressions */
import React from "react";
import { LinkProps } from "components/link";
import Link, { LinkProps } from "../../../src/components/link";
import { LinkComponent } from "../../../src/components/link/link-test.stories";
import { skipLink } from "../../locators/link/index";
import { keyCode } from "../../support/helper";
import { CHARACTERS } from "../../support/component-helper/constants";
import CypressMountWithProviders from "../../support/component-helper/cypress-mount";
import Box from "../../../src/components/box";
import { ICON } from "../../locators/locators";
import {
icon,
tooltipPreview,
Expand Down Expand Up @@ -72,6 +73,18 @@ context("Test for Link component", () => {
tooltipPreview().should("have.text", testCypress);
});

Copy link
Contributor

@Parsium Parsium Oct 6, 2023

Choose a reason for hiding this comment

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

question: wondering if these cypress tests would be necessary here, given we are testing the same styling with jest tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, technically we could probably get away with just having unit tests, but I thought it was worth just doubling up as we've done the same for other features in Link

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't really mind if you leave both unit and CI tests in tbh. However I do think a test story so chromatic can capture it would be best

it("when no children are passed, there should be no text decoration on the anchor element", () => {
CypressMountWithProviders(<Link icon="bin" href="www.sage.com" />);

link().find("a").should("have.css", "text-decoration-line", "none");
});

it("when no children are passed, link should have the inline display property", () => {
CypressMountWithProviders(<Link icon="bin" href="www.sage.com" />);

link().find(ICON).should("have.css", "display", "inline");
});

it.each([
"top",
"bottom",
Expand Down
33 changes: 32 additions & 1 deletion src/components/link/link-test.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ import { action } from "@storybook/addon-actions";
import { ICONS } from "../icon/icon-config";
import { LINK_ALIGNMENTS, LINK_POSITIONS, LINK_VARIANTS } from "./link.config";
import Link, { LinkProps } from "./link.component";
import Box from "../box";

export default {
title: "Link/Test",
includeStories: ["DefaultStory"],
includeStories: ["DefaultStory", "FlexContainer"],
parameters: {
info: { disable: true },
chromatic: {
Expand Down Expand Up @@ -94,6 +95,36 @@ DefaultStory.args = {
isDarkBackground: false,
};

export const FlexContainer = () => {
const link = (
<Box
display="flex"
flexDirection="row"
justifyContent="flex-end"
alignItems="center"
width="60px"
height="40px"
bg="red"
mx={5}
>
<Link icon="close" variant="neutral" />
</Box>
);
return (
<div
style={{
margin: "64px",
width: "fit-content",
padding: "8px",
}}
>
{link}
</div>
);
};

FlexContainer.parameters = { chromatic: { disableSnapshot: false } };

export const LinkComponent = (props: LinkProps) => {
return (
<div
Expand Down
26 changes: 26 additions & 0 deletions src/components/link/link.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,32 @@ describe("Link", () => {
});
});

describe("when a link is rendered with an icon and no children", () => {
beforeEach(() => {
wrapper = mount(<Link icon="home" href="www.sage.com" />);
});

it("there should be no text decoration on the anchor element", () => {
assertStyleMatch(
{
textDecoration: "none",
},
wrapper.find(StyledLink),
{ modifier: "a" }
);
});

it("link should have the inline display property", () => {
assertStyleMatch(
{
display: "inline",
},
wrapper.find(StyledLink),
{ modifier: `a > ${StyledIcon}` }
);
});
});

describe("when the `onKeyDown` event is triggered", () => {
let onClickFn: () => jest.Mock;
let onKeyDownFn: () => jest.Mock;
Expand Down
4 changes: 2 additions & 2 deletions src/components/link/link.style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,11 @@ const StyledLink = styled.span<StyledLinkProps & PrivateStyledLinkProps>`

a,
button {
text-decoration: underline;
text-decoration: ${hasContent ? "underline" : "none"};
${isMenuItem && "display: inline-block;"}

> ${StyledIcon} {
display: inline-block;
display: ${hasContent ? "inline-block" : "inline"};
position: relative;
vertical-align: middle;
${iconAlign === "left" &&
Expand Down
Loading