feat(portrait, profile): align with design system and reduce code complexity #6373

tomdavies73 · 2023-10-16T10:21:49Z

Proposed behaviour

Colour tokens, and background colour tokens have been aligned with the design system for Profile, and subsequently for Portrait. This ensures current colour contrast issues which have been observed in both components have been addressed. Also, when the email prop is passed on Portrait a Link component is now rendered with a mailto: href allowing users to send emails to the specified email addressed when clicked. Support has also been added for the text prop, which is additional text which appears under the email link, this is typically used for phone numbers, but can also be any other relevant read only text.

A new darkBackground variant has been added to Profile. This was already present in Portait but has been amended to represent new DS designs. This will allow consumers to use Profile or Portait in Ui which has a dark background.

Finally, the DS class all text on Profile to be optional, when previously in the component they were mandatory. To address this, conditional rendering has been implemented to determine if any text children are present, if true the Profile component will be rendered. If false, the Portrait component is rendered instead. However, additional guards have also been placed to make sure the name prop is passed if consumers do pass text children.

Code complexity has also been greatly reduced, with both .style files for each component being simplified, with the removal of unnecessary functions, types and interfaces. Resulting in a much cleaner structure and reduction in complexity. Also, both Portrait subcomponents portrait-initials and portrait-gravatar have been removed, with the formers functionality replaced with just rendering a div with centred text, and the latters functionality being added to the Portrait component.

Current behaviour

Currently, the Profile component is not aligned with the DS, and is missing some significant functionality and tokens, which result in some colour contrast issues which have been observed in our Playwright tests. Portrait is not in the DS technically, but we rely on Portrait for guidance on tokens etc. Portrait also supported a wide array of borders from dashed and its subcomponents were also overly complex, with two subcomponents and a style file which utilised unnecessary functions, types and interfaces. Resulting in a fairly complex component for what is only essentially a rounded or square image or <div>.

Now all gravatar functionality has been moved inside the component file, and the initials prop no longer renders an <img>, instead it renders a div with text centred.

Checklist

QA

Tested in CodeSandbox/storybook
Add new Cypress test coverage if required
Carbon implementation matches Design System/designs
UI Tests GitHub check reviewed if required

Additional context

Testing instructions

The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by codesandbox[bot].

codesandbox-ci · 2023-10-17T15:05:10Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 611c63e:

Sandbox	Source
carbon-quickstart	Configuration
carbon-quickstart-typescript	Configuration
carbon-quickstart	PR

robinzigmond · 2023-10-25T14:52:56Z

src/components/portrait/portrait.style.tsx

+    ::before {
+      font-size: ${({ size }) => PORTRAIT_SIZE_PARAMS[size].iconDimensions}px;
+    }
+`;


Suggested change

`;

}`;

suggestion: Without this I'm getting a syntax error highlighted by VSCode (from the ts-styled-plugin), which looks accurate to me. After changing it I can't see any obvious difference in styles - but to be honest I'm surprised it's working at all without it!

Good shout, i'll sort this and install the plugin too so I catch things like this going forward

no worries. For what it's worth, I don't remember installing that plugin and I can't see it listed in my VSCode extensions, so I'm not actually sure where it comes from 😅

EDIT: it might be from the vscode-styled-components extension I guess, which I do have

robinzigmond · 2023-10-25T14:58:26Z

src/components/portrait/portrait.component.tsx

-        shape={shape || defaultShape}
-        darkBackground={darkBackground}
-      />
+      <StyledIcon type={iconType} size={size} darkBackground={darkBackground} />


Suggested change

<StyledIcon type={iconType} size={size} darkBackground={darkBackground} />

<StyledIcon type={iconType} size={size} />

suggestion: it doesn't look like the darkBackground prop is used at all in the styled component, as this is now on the StyledPortraitContainer instead. So let's remove this.

robinzigmond · 2023-10-25T14:59:05Z

src/components/portrait/portrait.component.tsx

          size={size}
-          shape={shape || defaultShape}
-          initials={initials}
          darkBackground={darkBackground}


Suggested change

darkBackground={darkBackground}

suggestion: as above, this prop isn't used by the styled component any more

robinzigmond · 2023-10-25T15:00:01Z

src/components/portrait/portrait.style.tsx

-    }
-  }
+export const StyledPortraitInitials = styled.div<
+  Pick<StyledPortraitProps, "darkBackground" | "size">


Suggested change

Pick<StyledPortraitProps, "darkBackground" | "size">

Pick<StyledPortraitProps, "size">

suggestion: as pointed out in the component file, this styled component doesn't do anything with this prop so shouldn't even accept it

robinzigmond · 2023-10-25T15:00:26Z

src/components/portrait/portrait.style.tsx

-  type: IconType;
-}>`
+export const StyledIcon = styled(Icon)<
+  Pick<StyledPortraitProps, "darkBackground" | "size">


Suggested change

Pick<StyledPortraitProps, "darkBackground" | "size">

Pick<StyledPortraitProps, "size">

suggesion: as above

robinzigmond · 2023-10-25T15:58:12Z

src/components/profile/profile.component.tsx

@@ -52,45 +63,65 @@ export const Profile = ({
      return (
        <ProfileAvatarStyle
          src={src}


suggestion (non-blocking): the two different returned react nodes now have basically the same props - only src, gravatar and data-element are different. It might be worth creating an object with those common props and their values (src, darkBackground, alt, name, initials and size) and spreading it onto both of them. That also makes it easier for those reading the code to see what the differences are between the two cases.

Yeah, I agree with this, definitely something I should have seen sooner will make the component file a bit less complex 👍

robinzigmond · 2023-10-25T16:03:35Z

src/components/profile/profile.component.tsx

-        </ProfileEmailStyle>
-      </ProfileDetailsStyle>
-    );
+  invariant(


comment (non-blocking): I'd be cautious about using invariant here unless you have very clear requirements that consumers should absolutely not do this. If the condition fires the entire app will crash.

This isn't a breaking change (assuming consumers were using typescript), because both name and email were required props before - but even so I'd be personally more inclined to suggest this more gently, perhaps with a console.warn, because I don't personally see a good reason why you couldn't leave out name and just have the corresponding element be empty or not there.

Yeah fair point, having a name is pretty essential to the component as an email or text underneath really doesn't make much sense without it. I'll leave this one for the next reviewer to get their opinion if that's ok as i'm on the fence

yeah that's fair enough, I just thought I'd ask the question. If we're sure it makes no sense to have text/email with no name then I'm happy to keep this.

Yep agree invariant feels a little heavy handed here

robinzigmond · 2023-10-25T16:07:18Z

src/components/profile/profile.pw.tsx

@@ -59,58 +61,178 @@ test.describe("Prop checks for Profile component", () => {
    });
  });

-  ([


question: it looks like these tests, checking the appropriate width and height styles exist depending on the size prop, have been removed. Is there a good reason for that? Particularly as I see you've added tests for things like line-height, which I'm not saying not to do but seem a less important CSS property to test than width or height.

Good question, I felt these were redundant though as we're testing the Portait component here, when I feel there's already enough tests in that file for sizes, we actually cover every type of prop in portrait.pw.tsx whereas this previous test was only testing the initials prop

Essentially, I felt repeating the test here was redundant

I see, I'd missed that these were effectively testing portrait. Happy to leave this for the next reviewer to comment on, on the one hand I agree with you, but on the other the fact that profile sometimes renders portrait could be argued to be just an implementation detail, and that if we ever refactored that we'd still want to be sure profile was rendering at the correct size. (Mind you Chromatic should catch any changes there.) So there's an argument both ways - personally given the uncertainty I'd always rather have too many tests than too few, but as I said I'm happy to wait for another opinion and don't object to dropping this.

As long as we have some way of catching if this were to regress (ie passing size to Profile doesn't set Portrait) I don't mind where they are

robinzigmond · 2023-10-25T16:10:42Z

src/components/profile/profile.spec.tsx

-      it("returns empty string when name is a space", () => {
-        instance = shallow(<Profile name=" " email="foo@bar.com" />);
-        expect(instance.find(ProfileAvatarStyle).props().initials).toEqual("");
+      test("acronymize function generates correct acronym", () => {


comment: I'm not sure I like explicitly testing the acronymise function individually, particularly when that means exporting it from the component file. I see it as more of an implementation detail. Can we not test the same thing by just checking the text content of the appropriate element when various name values are provided as prop?

Agreed, I'll see if we can test something similar without exporting the function

robinzigmond · 2023-10-25T16:15:52Z

src/components/profile/profile.stories.mdx

@@ -51,6 +63,14 @@ To use an image, simply pass any valid image URL as a `src` prop.
  <Story name="src" story={stories.Src} />
 </Canvas>

+### Gravatar


question: probably one for the wider team, but even though this has always worked and we definitely shouldn't remove it as part of this PR, I'm not sure explicitly documenting the gravatar functionality for the first time at this stage is a good idea. I'm pretty sure we have mentioned removing/deprecating this feature (from Portrait, which would obviously also stop it working here).

Agree, i'll remove this story example

Yeah I agree, I think we can look to deprecate this soon so lets not document it and inadvertently encourage it's use

edleeks87 · 2023-10-27T10:12:50Z

src/components/portrait/portrait.style.tsx

-
-type StyledPortraitInitialsProps = PortraitSizeAndShape & {
-  initials?: string;
+  type: IconType;


Question: Is this used anywhere? I think you get this for free by using styled(Icon)

edleeks87 · 2023-10-27T10:15:10Z

src/components/portrait/portrait.style.tsx

  display: inline-block;
+  flex-shrink: 0;


Question: if this is only needed when in Profile should we not apply it there? I understand it's not doing anything in isolation but seems pointless to apply superfluous styles

edleeks87 · 2023-10-27T10:17:44Z

src/components/profile/profile.component.tsx

-        </ProfileEmailStyle>
-      </ProfileDetailsStyle>
-    );
+  invariant(


Yep agree invariant feels a little heavy handed here

edleeks87 · 2023-10-27T10:19:33Z

src/components/profile/profile.pw.tsx

@@ -59,58 +61,178 @@ test.describe("Prop checks for Profile component", () => {
    });
  });

-  ([


As long as we have some way of catching if this were to regress (ie passing size to Profile doesn't set Portrait) I don't mind where they are

edleeks87 · 2023-10-27T10:21:06Z

src/components/profile/profile.stories.mdx

@@ -51,6 +63,14 @@ To use an image, simply pass any valid image URL as a `src` prop.
  <Story name="src" story={stories.Src} />
 </Canvas>

+### Gravatar


Yeah I agree, I think we can look to deprecate this soon so lets not document it and inadvertently encourage it's use

edleeks87 · 2023-10-27T10:21:44Z

src/components/portrait/portrait.style.tsx


-export const StyledPortraitContainer = styled.div<StyledPortraitContainerProps>`
+export const StyledPortraitContainer = styled.div<
+  Pick<StyledPortraitProps, "darkBackground" | "size" | "shape"> & MarginProps


Suggested change

Pick<StyledPortraitProps, "darkBackground" | "size" | "shape"> & MarginProps

StyledPortraitProps & MarginProps

DipperTheDan

I have noticed a potential bug with this PR. We previously would only allow three characters to be displayed as initials in Profile and Portrait Even if you passed in more than three characters. Like this:

However in this PR, it now appears more than three will be displayed: Like this:

Is this behaviour expected?

Tom: Yeah just noticed we sliced the initials in the now deleted portrait-initials.component.tsx, i'll make sure we do the same with the new approach

DipperTheDan · 2023-10-30T13:21:20Z

src/components/portrait/portrait-initials.component.tsx

-      context.fillRect(0, 0, dimensions, dimensions);
-      context.fillStyle = tokens[textColor];
-      context.fillText(
-        initials.slice(0, 3).toUpperCase(),


reminder: You'll need this so the initials only render three, even if a million characters are passed in 😄

edleeks87 · 2023-11-01T16:51:51Z

src/components/profile/profile.component.tsx

-          {email}
-        </ProfileEmailStyle>
-      </ProfileDetailsStyle>
+  let useOfNoNameWarnTriggered = false;


comment: we should initialise this outside of the component so it's shared across all instances. It will be set to false for each component that renders.

edleeks87 · 2023-11-01T16:53:36Z

src/components/profile/profile.spec.tsx

+    jest.clearAllMocks();
+  });
+
+  describe("console warnings", () => {


comment: we should test that it only runs the logger once even when multiple Profile components are rendered

DipperTheDan · 2023-11-02T08:53:04Z

src/components/portrait/portrait.pw.tsx

@@ -47,11 +50,20 @@ const portraitSizes = PORTRAIT_SIZES.map((size) => [
 ]);

 test.describe("Prop checks for Portrait component", () => {
+  ["SPM", "JM", "AR", "MJ"].forEach((passInitials) => {


suggestion/question(non-blocking): Do you think it's worth having a test that passes four initials but checks that only three have been rendered? Thinking behind it is if in future someone else comes to refactor these again, we have a test to cover that functionality.

Yeah i've covered this in a unit test but I don't mind adding a Playwright test too

Actually, a unit test should be enough for this for now. If we discover in future, that some sort of user interaction can cause more than three to appear in the browser, then it would warrant a Playwright test.

Just added and amended some playwright tests anyways to test the functionality, can't hurt 👍

…plexity aligns components with the sage design system, to ensure consumers can use the component with the expected styling and functionality. This includes the addition of a new dark background variant on `Profile`, as well as ensuring the same variant is consistent on the sub-component `Portrait`. This aim for consistency has also been applied to all use of colour and background colour tokens, ensuring they are aligned with the design system, which also removes some colour contrast issues which were present due to the use of previous colours which are no longer recommended by the design system. As well as styling changes, the `text` prop has been added, which gives consumers the ability to pass some read-only text underneath where the user's email is typically rendered. This is commonly used for phone numbers etc. but can be used for any other added information or context needed for a user's profile. Also, in regard to the rendered email, this is now rendered as the `Link` carbon component, with some additional colour changes to ensure proper colour contrast is achieved when used in the aforementioned dark background variant and also now has a mailto href which is standard for emails in user profiles. Code complexity has also been greatly reduced throughout both components, and their sub- components. This has greatly improved code simplicity and consistency, particularly when initials are rendered in `Portrait`, a new approach has been used which renders a div instead of an image, eradicating some pixel abnormalities when the prop was used. fix #6191

…nd remove invalid tooltip tests

carbonci · 2023-11-15T17:00:05Z

🎉 This PR is included in version 123.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

tomdavies73 added the DO NOT MERGE label Oct 16, 2023

tomdavies73 self-assigned this Oct 16, 2023

tomdavies73 requested review from a team as code owners October 16, 2023 10:21

tomdavies73 marked this pull request as draft October 16, 2023 10:22

tomdavies73 force-pushed the FE-6214 branch from 6d89c2a to 4a363cd Compare October 17, 2023 15:02

tomdavies73 force-pushed the FE-6214 branch 6 times, most recently from 6805001 to 05c42fd Compare October 24, 2023 15:01

tomdavies73 added Pending Review Pending QA Pending UX QA and removed DO NOT MERGE labels Oct 24, 2023

robinzigmond self-requested a review October 25, 2023 14:28

robinzigmond reviewed Oct 25, 2023

View reviewed changes

edleeks87 reviewed Oct 27, 2023

View reviewed changes

DipperTheDan reviewed Oct 30, 2023

View reviewed changes

tomdavies73 force-pushed the FE-6214 branch 3 times, most recently from 5707cfa to 71ae631 Compare November 1, 2023 15:55

edleeks87 reviewed Nov 1, 2023

View reviewed changes

DipperTheDan reviewed Nov 2, 2023

View reviewed changes

tomdavies73 force-pushed the FE-6214 branch from 71ae631 to e611fa1 Compare November 2, 2023 15:30

edleeks87 previously approved these changes Nov 2, 2023

View reviewed changes

DipperTheDan previously approved these changes Nov 3, 2023

View reviewed changes

DipperTheDan marked this pull request as ready for review November 3, 2023 09:16

DipperTheDan added Passed Review Squad Front End and removed Pending Review labels Nov 3, 2023

tomdavies73 dismissed stale reviews from DipperTheDan and edleeks87 via cb4c870 November 3, 2023 15:57

tomdavies73 force-pushed the FE-6214 branch from e611fa1 to cb4c870 Compare November 3, 2023 15:57

harpalsingh added Passed UX QA and removed Pending UX QA labels Nov 6, 2023

tomdavies73 force-pushed the FE-6214 branch from cb4c870 to f7b4fef Compare November 14, 2023 14:12

edleeks87 approved these changes Nov 14, 2023

View reviewed changes

DipperTheDan approved these changes Nov 14, 2023

View reviewed changes

ZhuoyuJin approved these changes Nov 15, 2023

View reviewed changes

ZhuoyuJin added Passed QA and removed Pending QA labels Nov 15, 2023

tomdavies73 added 2 commits November 15, 2023 16:20

test(portrait): remove disabled color contrast rules in fixed tests a…

611c63e

…nd remove invalid tooltip tests

tomdavies73 force-pushed the FE-6214 branch from f7b4fef to 611c63e Compare November 15, 2023 16:20

tomdavies73 merged commit f4cba31 into master Nov 15, 2023
29 checks passed

tomdavies73 deleted the FE-6214 branch November 15, 2023 16:55

carbonci added the released label Nov 15, 2023

	<StyledIcon type={iconType} size={size} darkBackground={darkBackground} />
	<StyledIcon type={iconType} size={size} />

	Pick<StyledPortraitProps, "darkBackground" \| "size">
	Pick<StyledPortraitProps, "size">

@@ @@ -59,58 +61,178 @@ test.describe("Prop checks for Profile component", () => { @@
                   });
                 });
-                ([

	Pick<StyledPortraitProps, "darkBackground" \| "size" \| "shape"> & MarginProps
	StyledPortraitProps & MarginProps

feat(portrait, profile): align with design system and reduce code complexity #6373

feat(portrait, profile): align with design system and reduce code complexity #6373

Conversation

tomdavies73 commented Oct 16, 2023 • edited Loading

Proposed behaviour

Current behaviour

Checklist

QA

Additional context

Testing instructions

codesandbox-ci bot commented Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinzigmond Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DipperTheDan left a comment • edited by tomdavies73 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonci commented Nov 15, 2023

tomdavies73 commented Oct 16, 2023 •

edited

Loading

codesandbox-ci bot commented Oct 17, 2023 •

edited

Loading

robinzigmond Oct 26, 2023 •

edited

Loading

DipperTheDan left a comment •

edited by tomdavies73

Loading