From cb4c87063ffc66c50a25d3ad38fc6d3a0ae7dab8 Mon Sep 17 00:00:00 2001 From: "tom.davies" Date: Wed, 1 Nov 2023 13:01:28 +0000 Subject: [PATCH] chore: address comments from review --- .../portrait/portrait.component.tsx | 12 +- src/components/portrait/portrait.pw.tsx | 20 +- src/components/portrait/portrait.spec.tsx | 2 - src/components/portrait/portrait.stories.tsx | 8 +- src/components/portrait/portrait.style.tsx | 13 +- src/components/profile/component.test-pw.tsx | 6 +- .../profile/profile-test.stories.tsx | 1 + src/components/profile/profile.component.tsx | 44 ++- src/components/profile/profile.pw.tsx | 3 +- src/components/profile/profile.spec.tsx | 342 ++++++++---------- src/components/profile/profile.stories.mdx | 8 - src/components/profile/profile.stories.tsx | 8 - src/components/profile/profile.style.ts | 6 +- 13 files changed, 210 insertions(+), 263 deletions(-) diff --git a/src/components/portrait/portrait.component.tsx b/src/components/portrait/portrait.component.tsx index aa51caef39..9af7f8cd44 100644 --- a/src/components/portrait/portrait.component.tsx +++ b/src/components/portrait/portrait.component.tsx @@ -108,18 +108,12 @@ const Portrait = ({ }; const renderComponent = () => { - let portrait = ( - - ); + let portrait = ; if (initials) { portrait = ( - - {initials.toUpperCase()} + + {initials.slice(0, 3).toUpperCase()} ); } diff --git a/src/components/portrait/portrait.pw.tsx b/src/components/portrait/portrait.pw.tsx index 9fbf8d787c..2072b0a71e 100644 --- a/src/components/portrait/portrait.pw.tsx +++ b/src/components/portrait/portrait.pw.tsx @@ -61,6 +61,22 @@ test.describe("Prop checks for Portrait component", () => { }); }); + [ + ["SPMMMMM", "SPM"], + ["JMMMMM", "JMM"], + ["ARRRR", "ARR"], + ["MJJJ", "MJJ"], + ].forEach(([passInitials, maxInitials]) => { + test(`should render with a maximum of three initials when passed initials are ${passInitials} `, async ({ + mount, + page, + }) => { + await mount(); + + await expect(portraitInitials(page)).toHaveText(maxInitials); + }); + }); + test("should render with gravatar prop", async ({ mount, page }) => { await mount(); @@ -227,8 +243,6 @@ test.describe("Prop checks for Portrait component", () => { "background-color" ); - console.log(backgroundColorTokens); - expect(backgroundColorTokens.toString()).toBe(tokenVal); await expect(portraitPreview(page)).toHaveCSS( "background-color", @@ -255,8 +269,6 @@ test.describe("Prop checks for Portrait component", () => { "color" ); - console.log(colorTokens); - expect(colorTokens.toString()).toBe(tokenVal); await expect(portraitPreview(page)).toHaveCSS("color", color); }); diff --git a/src/components/portrait/portrait.spec.tsx b/src/components/portrait/portrait.spec.tsx index 724952e54a..70a6df7117 100644 --- a/src/components/portrait/portrait.spec.tsx +++ b/src/components/portrait/portrait.spec.tsx @@ -54,7 +54,6 @@ describe("PortraitComponent", () => { overflow: "hidden", border: "1px solid var(--colorsUtilityReadOnly600)", display: "inline-block", - flexShrink: "0", }, wrapper.find(StyledPortraitContainer) ); @@ -411,7 +410,6 @@ describe("PortraitComponent", () => { const expectedProps = { children: "AB", size: "M", - darkBackground: false, }; const testSuccess = (element: JSX.Element) => diff --git a/src/components/portrait/portrait.stories.tsx b/src/components/portrait/portrait.stories.tsx index f966f54741..bc58b4b065 100644 --- a/src/components/portrait/portrait.stories.tsx +++ b/src/components/portrait/portrait.stories.tsx @@ -35,11 +35,11 @@ export const WithTooltip: ComponentStory = () => ( export const Sizes: ComponentStory = () => { return ( - <> + {(["XS", "S", "M", "ML", "L", "XL", "XXL"] as const).map((size) => ( ))} - + ); }; @@ -61,10 +61,10 @@ export const DarkBackground: ComponentStory = () => ( ); export const WithMargin: ComponentStory = () => ( - <> + - + ); diff --git a/src/components/portrait/portrait.style.tsx b/src/components/portrait/portrait.style.tsx index d74a6847f2..45f7592632 100644 --- a/src/components/portrait/portrait.style.tsx +++ b/src/components/portrait/portrait.style.tsx @@ -3,7 +3,7 @@ import styled from "styled-components"; import { margin, MarginProps } from "styled-system"; import BaseTheme from "../../style/themes/base"; -import Icon, { IconType } from "../icon"; +import Icon from "../icon"; import { PortraitSizes, PortraitShapes } from "./portrait.component"; import { PORTRAIT_SIZE_PARAMS } from "./portrait.config"; import profileConfigSizes from "../profile/profile.config"; @@ -12,12 +12,11 @@ type StyledPortraitProps = { darkBackground?: boolean; size: PortraitSizes; shape?: PortraitShapes; - type: IconType; onClick?: (ev: React.MouseEvent) => void; }; export const StyledPortraitInitials = styled.div< - Pick + Pick >` font-weight: bold; font-size: ${({ size }) => profileConfigSizes[size].initialSize}; @@ -41,9 +40,7 @@ export const StyledCustomImg = styled.img` // && is used here to increase the specificity // eslint-disable-next-line @typescript-eslint/no-unused-vars -export const StyledIcon = styled(Icon)< - Pick ->` +export const StyledIcon = styled(Icon)>` && { color: inherit; height: inherit; @@ -52,10 +49,11 @@ export const StyledIcon = styled(Icon)< ::before { font-size: ${({ size }) => PORTRAIT_SIZE_PARAMS[size].iconDimensions}px; } + } `; export const StyledPortraitContainer = styled.div< - Pick & MarginProps + StyledPortraitProps & MarginProps >` color: ${({ darkBackground }) => darkBackground @@ -72,7 +70,6 @@ export const StyledPortraitContainer = styled.div< shape === "square" ? "0px" : "var(--borderRadiusCircle)"}; border: 1px solid var(--colorsUtilityReadOnly600); display: inline-block; - flex-shrink: 0; ${({ onClick }) => onClick && "cursor: pointer"} ${margin} `; diff --git a/src/components/profile/component.test-pw.tsx b/src/components/profile/component.test-pw.tsx index 30a431a9c8..6cb2317cae 100644 --- a/src/components/profile/component.test-pw.tsx +++ b/src/components/profile/component.test-pw.tsx @@ -1,11 +1,11 @@ import React from "react"; import Profile, { ProfileProps } from "."; -const EmptyProfileComponent = (props: Partial) => { +export const EmptyProfileComponent = (props: Partial) => { return ; }; -const ProfileComponent = (props: Partial) => { +export const ProfileComponent = (props: Partial) => { return ( ) => { /> ); }; - -export { EmptyProfileComponent, ProfileComponent }; diff --git a/src/components/profile/profile-test.stories.tsx b/src/components/profile/profile-test.stories.tsx index 4b62b5cdf6..4727a82eec 100644 --- a/src/components/profile/profile-test.stories.tsx +++ b/src/components/profile/profile-test.stories.tsx @@ -32,6 +32,7 @@ DefaultStory.story = { initials: "JS", size: PROFILE_SIZES[0], name: "John Smith", + text: "Some other text about John here", src: "", }, }; diff --git a/src/components/profile/profile.component.tsx b/src/components/profile/profile.component.tsx index fbb2bc4377..63262623a1 100644 --- a/src/components/profile/profile.component.tsx +++ b/src/components/profile/profile.component.tsx @@ -1,5 +1,4 @@ import React from "react"; -import invariant from "invariant"; import tagComponent from "../../__internal__/utils/helpers/tags/tags"; import { @@ -13,13 +12,15 @@ import { import { filterStyledSystemMarginProps } from "../../style/utils"; import { ProfileSize } from "./profile.config"; -export function acronymize(str?: string) { +function acronymize(str?: string) { if (!str) return ""; const matches = str.match(/\b\w/g); if (!matches) return ""; return matches.join(""); } +let useOfNoNameWarnTriggered = false; + export interface ProfileProps { /** [Legacy] A custom class name for the component */ className?: string; @@ -55,7 +56,15 @@ export const Profile = ({ }: ProfileProps) => { const getInitials = () => { if (initials) return initials; - return acronymize(name); + return acronymize(name).slice(0, 3).toUpperCase(); + }; + + const commonAvatarProps = { + darkBackground, + alt, + name, + initials: getInitials(), + size, }; const avatar = () => { @@ -63,32 +72,21 @@ export const Profile = ({ return ( ); } - return ( - - ); + return ; }; - invariant( - !(!name && (email || text)), - "The `email` or `text` prop cannot be used without the `name` prop." + - " Please use the `name` prop as well as `email` or `text`." - ); + if (!useOfNoNameWarnTriggered && !name && (email || text)) { + useOfNoNameWarnTriggered = true; + console.warn( + "[WARNING] The `email` or `text` prop should not be used without the `name` prop in `Profile`." + + " Please use the `name` prop as well as `email` or `text`." + ); + } const children = () => { if (name) diff --git a/src/components/profile/profile.pw.tsx b/src/components/profile/profile.pw.tsx index 4d6e6e0ee7..9b7af8b902 100644 --- a/src/components/profile/profile.pw.tsx +++ b/src/components/profile/profile.pw.tsx @@ -62,11 +62,12 @@ test.describe("Prop checks for Profile component", () => { }); [ + ["World Wide Web Web", "WWW"], ["World Wide Web", "WWW"], ["John Doe", "JD"], ["Foo", "F"], ].forEach(([name, initials]) => { - test(`should render with correct initials when nname prop is passed as ${name} and no other icon, src or gravatar is passed`, async ({ + test(`should render with correct initials when name prop is passed as ${name} and no other icon, src or gravatar is passed`, async ({ mount, page, }) => { diff --git a/src/components/profile/profile.spec.tsx b/src/components/profile/profile.spec.tsx index b9780bf975..83141d29a7 100644 --- a/src/components/profile/profile.spec.tsx +++ b/src/components/profile/profile.spec.tsx @@ -2,7 +2,7 @@ import React from "react"; import { shallow, mount, ShallowWrapper, ReactWrapper } from "enzyme"; import MD5 from "crypto-js/md5"; -import Profile, { acronymize } from "./profile.component"; +import Profile from "./profile.component"; import { elementsTagTest, rootTagTest, @@ -17,6 +17,7 @@ import { } from "./profile.style"; import { StyledCustomImg, + StyledPortraitContainer, StyledPortraitInitials, } from "../portrait/portrait.style"; import { StyledLink } from "../link/link.style"; @@ -29,6 +30,43 @@ import profileConfigSizes, { ProfileSize } from "./profile.config"; describe("Profile", () => { let instance: ShallowWrapper | ReactWrapper; + let loggerSpy: jest.SpyInstance | jest.Mock; + + beforeEach(() => { + loggerSpy = jest.spyOn(console, "warn"); + instance = mount(); + }); + + afterEach(() => { + loggerSpy.mockRestore(); + instance.unmount(); + }); + + afterAll(() => { + loggerSpy.mockClear(); + jest.clearAllMocks(); + }); + + describe("console warning", () => { + const warningMessage = + "[WARNING] The `email` or `text` prop should not be used without the `name` prop in `Profile`." + + " Please use the `name` prop as well as `email` or `text`."; + + it("validates a warning is logged in the console once", () => { + mount( + <> + + + + ); + + expect(loggerSpy).toHaveBeenCalledWith(warningMessage); + + expect(loggerSpy).toHaveBeenCalledTimes(1); + + loggerSpy.mockRestore(); + }); + }); describe("render", () => { beforeEach(() => { @@ -43,34 +81,48 @@ describe("Profile", () => { }); describe("initials", () => { - it("calculates the initials when not provided", () => { - instance = shallow(); + it("calculates the correct initial when only a name is provided", () => { + instance = shallow(); expect(instance.find(ProfileAvatarStyle).props().initials).toEqual( "JD" ); }); - it("returns an empty string if no name is found", () => { - instance = shallow(); - expect(instance.find(ProfileAvatarStyle).props().initials).toEqual(""); - }); + it.each([ + ["John Doe", "JD"], + ["John Dan Doe", "JDD"], + ])( + "calculates the correct initials when only a name is provided", + (name, initials) => { + instance = shallow(); + expect(instance.find(ProfileAvatarStyle).props().initials).toEqual( + initials + ); + } + ); - test("acronymize function generates correct acronym", () => { - const input = "World Wide Web"; - const result = acronymize(input); - expect(result).toEqual("WWW"); - }); + it.each([ + "John David Daniel Doe", + "John David Daniel Damien Doe", + "John David Daniel Damien Dack Doe", + ])( + "calculates the correct maximum initials of three, when only a long name is provided", + (name) => { + instance = shallow(); + expect(instance.find(ProfileAvatarStyle).props().initials).toEqual( + "JDD" + ); + } + ); - test("acronymize function returns empty string if passed string is empty", () => { - const input = ""; - const result = acronymize(input); - expect(result).toEqual(""); + it("returns an empty initials string if no name is provided", () => { + instance = shallow(); + expect(instance.find(ProfileAvatarStyle).props().initials).toEqual(""); }); - test("acronymize function returns empty string if passed string as no word boundaries are identified", () => { - const input = "!@£$%"; - const result = acronymize(input); - expect(result).toEqual(""); + it("returns an empty initials string if provided name has no identifiable word boundaries", () => { + instance = shallow(); + expect(instance.find(ProfileAvatarStyle).props().initials).toEqual(""); }); }); @@ -113,107 +165,6 @@ describe("Profile", () => { instance = mount(); expect(instance.find(StyledPortraitInitials).text()).toBe("JD"); }); - - it("validates an invariant is thrown if `name` is not passed but `email` is", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => instance.setProps({ email: "john@doe.com" })).toThrow( - "The `email` or `text` prop cannot be used without the `name` prop." + - " Please use the `name` prop as well as `email` or `text`." - ); - - consoleSpy.mockRestore(); - }); - - it("validates an invariant is thrown if `name` is not passed but `text` is", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => - instance.setProps({ text: "Some other text about John here" }) - ).toThrow( - "The `email` or `text` prop cannot be used without the `name` prop." + - " Please use the `name` prop as well as `email` or `text`." - ); - - consoleSpy.mockRestore(); - }); - - it("validates an invariant is thrown if `name` is not passed but `text` and `email` are", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => - instance.setProps({ - text: "Some other text about John here", - email: "john@doe.com", - }) - ).toThrow( - "The `email` or `text` prop cannot be used without the `name` prop." + - " Please use the `name` prop as well as `email` or `text`." - ); - - consoleSpy.mockRestore(); - }); - - it("validates an invariant is not thrown if `name` is passed and `email` is not", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => - instance.setProps({ - name: "John Doe", - text: "Some other text about John here", - }) - ).not.toThrow(); - - consoleSpy.mockRestore(); - }); - - it("validates an invariant is not thrown if `name` is passed and `text` is not", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => - instance.setProps({ name: "John Doe", email: "john@doe.com" }) - ).not.toThrow(); - - consoleSpy.mockRestore(); - }); - - it("validates an invariant is not thrown if `name` is passed and both `text` and `email` are not", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => instance.setProps({ name: "John Doe" })).not.toThrow(); - - consoleSpy.mockRestore(); - }); - - it("validates an invariant is not thrown if `name` is passed and both `text` and `email` are", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => - instance.setProps({ - name: "John Doe", - email: "john@doe.com", - text: "Some other text about John here", - }) - ).not.toThrow(); - - consoleSpy.mockRestore(); - }); - - it("validates an invariant is not thrown if `name`,`text` and `email` are all not passed", () => { - const consoleSpy = jest.spyOn(console, "error"); - consoleSpy.mockImplementation(() => {}); - - expect(() => instance.setProps({})).not.toThrow(); - - consoleSpy.mockRestore(); - }); }); }); @@ -233,12 +184,21 @@ describe("Profile", () => { whiteSpace: "nowrap", display: "flex", flexDirection: "row", - flexShrink: "0", }, wrapper.find(ProfileStyle) ); }); + it("applies expected styles to Portrait container", () => { + assertStyleMatch( + { + flexShrink: "0", + }, + wrapper.find(ProfileStyle), + { modifier: `${StyledPortraitContainer}` } + ); + }); + it("applies expected styles to Profile name", () => { assertStyleMatch( { @@ -396,86 +356,86 @@ describe("Profile", () => { elementsTagTest(wrapper, ["email", "name"]); }); }); -}); -describe("src", () => { - it('should render a user image when a "src" prop is provided', () => { - const wrapper = shallow( - - ); - expect(wrapper.find(ProfileAvatarStyle).props()["data-element"]).toEqual( - "user-image" - ); - }); + describe("src", () => { + it('should render a user image when a "src" prop is provided', () => { + const wrapper = shallow( + + ); + expect(wrapper.find(ProfileAvatarStyle).props()["data-element"]).toEqual( + "user-image" + ); + }); - it("alt text on user image should be provided via alt prop", () => { - const wrapper = mount( - - ); - expect(wrapper.find(StyledCustomImg).prop("alt")).toBe("John V Doe"); - }); + it("alt text on user image should be provided via alt prop", () => { + const wrapper = mount( + + ); + expect(wrapper.find(StyledCustomImg).prop("alt")).toBe("John V Doe"); + }); - it("alt text on user image should be the name passed, if no alt prop is provided", () => { - const wrapper = mount( - - ); - expect(wrapper.find(StyledCustomImg).prop("alt")).toEqual("John Doe"); + it("alt text on user image should be the name passed, if no alt prop is provided", () => { + const wrapper = mount( + + ); + expect(wrapper.find(StyledCustomImg).prop("alt")).toEqual("John Doe"); + }); }); -}); -describe("gravatar", () => { - const gravatarEmail = "chris.barber@sage.com"; - const base = "https://www.gravatar.com/avatar/"; - const hash = MD5(gravatarEmail); - const dimensions = 40; - const expectedSrc = `${base}${hash}?s=${dimensions}&d=404`; + describe("gravatar", () => { + const gravatarEmail = "chris.barber@sage.com"; + const base = "https://www.gravatar.com/avatar/"; + const hash = MD5(gravatarEmail); + const dimensions = 40; + const expectedSrc = `${base}${hash}?s=${dimensions}&d=404`; - it("should render a gravatar if passed email matches gravatar account", () => { - const wrapper = mount( - - ); + it("should render a gravatar if passed email matches gravatar account", () => { + const wrapper = mount( + + ); - expect(wrapper.find("img").prop("src")).toEqual(expectedSrc); - }); + expect(wrapper.find("img").prop("src")).toEqual(expectedSrc); + }); - it("alt text on gravatar should be provided via alt prop", () => { - const wrapper = mount( - - ); + it("alt text on gravatar should be provided via alt prop", () => { + const wrapper = mount( + + ); - expect(wrapper.find("img").prop("alt")).toEqual("Mr Chris Barber"); - }); + expect(wrapper.find("img").prop("alt")).toEqual("Mr Chris Barber"); + }); - it("alt text on user image should be the name passed if no alt prop is provided", () => { - const wrapper = mount( - - ); + it("alt text on user image should be the name passed if no alt prop is provided", () => { + const wrapper = mount( + + ); - expect(wrapper.find("img").prop("alt")).toEqual("Chris Barber"); + expect(wrapper.find("img").prop("alt")).toEqual("Chris Barber"); + }); }); -}); -describe("styled-system", () => { - testStyledSystemMargin((props) => ( - - )); + describe("styled-system", () => { + testStyledSystemMargin((props) => ( + + )); + }); }); diff --git a/src/components/profile/profile.stories.mdx b/src/components/profile/profile.stories.mdx index dfc864c64a..129a4aeced 100644 --- a/src/components/profile/profile.stories.mdx +++ b/src/components/profile/profile.stories.mdx @@ -63,14 +63,6 @@ To use an image, simply pass any valid image URL as a `src` prop. -### Gravatar - -Profile also supports `gravatar`, to use it simply pass your email address registered with Gravatar as the `email` prop. - - - - - ### Sizes diff --git a/src/components/profile/profile.stories.tsx b/src/components/profile/profile.stories.tsx index 6f47f319e8..71d029fe3c 100644 --- a/src/components/profile/profile.stories.tsx +++ b/src/components/profile/profile.stories.tsx @@ -43,14 +43,6 @@ export const Src: ComponentStory = () => ( /> ); -export const Gravatar: ComponentStory = () => ( - -); - export const Sizes: ComponentStory = () => { return ( <> diff --git a/src/components/profile/profile.style.ts b/src/components/profile/profile.style.ts index d57fbdf93a..28da7868a9 100644 --- a/src/components/profile/profile.style.ts +++ b/src/components/profile/profile.style.ts @@ -5,6 +5,7 @@ import Portrait from "../portrait"; import baseTheme from "../../style/themes/base"; import profileConfigSizes, { ProfileSize } from "./profile.config"; import Link from "../link"; +import { StyledPortraitContainer } from "../portrait/portrait.style"; interface ProfileSProps { size?: ProfileSize; @@ -40,8 +41,11 @@ const ProfileStyle = styled.div< darkBackground ? "var(--colorsUtilityYin090)" : "transparent"}; display: flex; flex-direction: row; - flex-shrink: 0; ${margin} + + ${StyledPortraitContainer} { + flex-shrink: 0; + } `; const ProfileDetailsStyle = styled.div>`