Feature: APP-2612 - Implement Avatar component #49

Fabricevladimir · 2023-11-23T17:05:47Z

Description

adds the avatar component

Task: APP-2612

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

I have selected the correct base branch.
I have performed a self-review of my own code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
Any dependent changes have been merged and published in downstream modules.
I ran all tests with success and extended them if necessary.
I have updated the CHANGELOG.md file in the root folder of the package after the [UPCOMING] title and before the latest version.
I have tested my code on the test network.

Fabricevladimir · 2023-11-28T15:20:10Z

src/styles/primitives/colors.css

+  --ods-gradient-tl-primary-500-to-800: linear-gradient(
+    135deg,
+    var(--ods-color-primary-500),
+    var(--ods-color-primary-800)
+  );
+
+  --ods-gradient-neutral-900: 31 41 51;
+  --ods-gradient-tl-neutral-900: linear-gradient(
+    135deg,
+    rgb(var(--ods-gradient-neutral-900) / 0.24),
+    rgb(var(--ods-gradient-neutral-900) / 0.96)
+  );


@RuggeroCino, I ran into an interesting little issue where you can either define color stops for the gradients or go all the way and define the whole gradient as a background image. I went for the latter option because it made more sense in terms of on the fly styling, but it does have its drawbacks. Here, for example, I was unable to find a way to add the transparency using the custom ODS color variable as long as that variable was defined as a hex code (no string concatenation in normal css 🤷🏾‍♂️). I'm not at all happy with this solution; what are your thoughts? Right now it's either we define the colors as RGB values or hardcode the gradient stops. Thoughts?

I see, CSS custom properties are hard to use with opacity :/
I would discuss this with Selim and see if we really need to use opacity for this gradient, if not we could use a lighter neutral color and have something similar to the primary 500 to 800. Otherwise let's discuss about the alternatives

cgero-eth

Well done @Fabricevladimir! I left some comments

cgero-eth · 2023-11-29T10:45:15Z

package.json

@@ -45,6 +45,7 @@
    "@radix-ui/react-progress": "^1.0.0",
    "classnames": "^2.3.0",
    "react": "^18.2.0",
+    "react-blockies": "^1.4.1",


Let's relax the version of the react-blockies dependency to ^1.0.0

cgero-eth · 2023-11-29T10:48:03Z

src/components/avatars/avatar/avatar.tsx

+import type React from 'react';
+import { useEffect, useState, type HTMLAttributes } from 'react';
+import Blockies from 'react-blockies';
+


I would either enforce this new space between the third party imports and the relative ones through a eslint rule or just remove this new line to be aligned across all components

cgero-eth · 2023-11-29T11:37:56Z

src/components/avatars/avatarIcon/avatarIcon.test.tsx

+        render(createTestComponent());
+        expect(screen.getByTestId(IconType.ADD)).toBeInTheDocument();


Let's not rely on the properties defined on the createTestComponent function, the ones set there are only used to be able to render a functional component. If we do so, we will end with having multiple tests coupled with each other. Instead let's specify them on the tests like:

const icon = IconType.ADD; render(createTestComponent({ icon }); expect(screen.getByTestId(icon)).toBeInTheDocument();

cgero-eth · 2023-11-29T11:42:51Z

src/components/avatars/avatar/fallbackAvatar.tsx

@@ -0,0 +1,13 @@
+import { type HTMLAttributes } from 'react';


As the component doesn't really have some specific logic, I would just combine this with the Avatar component. What do you think?

cgero-eth · 2023-11-29T11:48:56Z

src/components/avatars/avatar/avatar.test.tsx

+
+import { Avatar, type IAvatarProps } from './avatar';
+
+const etherIcon = 'https://assets.coingecko.com/coins/images/279/large/ethereum.png';


Let's move this to the test using it to avoid shared variables between tests. Also, right now the test might randomly fail if for some reason the asset url is not valid anymore. I'd suggest to try the solution posted here to fire the load and error events on the test instead

cgero-eth · 2023-11-29T11:56:53Z

src/components/avatars/avatar/avatar.tsx

+     * // Results in the fallback 'Al' due to a single-word copy.
+     * 'Alice'
+     */
+    src?: string;


We are delegating the logic of checking if a dao/user has a valid image/profile-image set to the components using this component. Let's sync if we can improve this by adding for instance a fallback property.

cgero-eth · 2023-11-29T13:59:44Z

src/utils/addresssUtils/addressUtils.ts

+    private addressRegex = /^0x[a-fA-F0-9]{40}$/;
+
+    isAddress(value: string | null | undefined): boolean {
+        if (!value || typeof value !== 'string') {


If we already specify that value is either string or null/undefined, we don't need to check that it is actually a string, we can simply write:

Suggested change

if (!value || typeof value !== 'string') {

if (value == null) {

cgero-eth · 2023-11-29T14:01:47Z

src/components/avatars/avatar/avatar.tsx

+    );
+};
+
+Avatar.displayName = 'Avatar';


The component display name is set automatically, right?

Fabricevladimir · 2024-01-22T11:22:51Z

Reopening new PR to make use of radix-ui's avatar primitive

Fabricevladimir marked this pull request as ready for review November 28, 2023 11:37

Fabricevladimir changed the title ~~WIP - Feature: APP-2612 - Implement Avatar component~~ Feature: APP-2612 - Implement Avatar component Nov 28, 2023

Fabricevladimir commented Nov 28, 2023

View reviewed changes

Fabricevladimir added 9 commits November 29, 2023 07:11

avatar icon test

796f41b

added react blockies

0248481

address utils

172a939

avatar component

5285f17

avatar tests

0248952

avatar tests and other stories

fc01c70

fix test and updated changelog

3aa875f

avatar updates and revert faulty test fix

7288d2b

avatar gradients

6e2912c

Fabricevladimir force-pushed the f/avatars branch from b835005 to 6e2912c Compare November 29, 2023 12:12

cgero-eth requested changes Nov 29, 2023

View reviewed changes

Fabricevladimir marked this pull request as draft November 30, 2023 11:10

Fabricevladimir closed this Jan 22, 2024

cgero-eth deleted the f/avatars branch July 23, 2024 08:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: APP-2612 - Implement Avatar component #49

Feature: APP-2612 - Implement Avatar component #49

Fabricevladimir commented Nov 23, 2023 •

edited by jira bot

Loading

Fabricevladimir Nov 28, 2023

cgero-eth Nov 29, 2023

cgero-eth left a comment

cgero-eth Nov 29, 2023

cgero-eth Nov 29, 2023

cgero-eth Nov 29, 2023

cgero-eth Nov 29, 2023

cgero-eth Nov 29, 2023

cgero-eth Nov 29, 2023

cgero-eth Nov 29, 2023

cgero-eth Nov 29, 2023

Fabricevladimir commented Jan 22, 2024

		render(createTestComponent());
		expect(screen.getByTestId(IconType.ADD)).toBeInTheDocument();

		@@ -0,0 +1,13 @@
		import { type HTMLAttributes } from 'react';


		import { Avatar, type IAvatarProps } from './avatar';

		const etherIcon = 'https://assets.coingecko.com/coins/images/279/large/ethereum.png';

	if (!value \|\| typeof value !== 'string') {
	if (value == null) {

Feature: APP-2612 - Implement Avatar component #49

Feature: APP-2612 - Implement Avatar component #49

Conversation

Fabricevladimir commented Nov 23, 2023 • edited by jira bot Loading

Description

Type of change

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgero-eth left a 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

Fabricevladimir commented Jan 22, 2024

Fabricevladimir commented Nov 23, 2023 •

edited by jira bot

Loading