Draft: Icon with activity sub-icon #654

camerow · 2024-11-25T22:37:03Z

Work in progress on adding an icon with activity sub icon.

Need to fix some shadow issues, but overall it is nearly done.

camerow · 2024-11-25T22:44:00Z

apps/mobile/src/components/widgets/accounts/components/cards/add-account-card.tsx

  return (
    <AccountCardLayout
      onPress={onPress}
-      icon={<PlusIcon />}
+      icon={<PlusIcon color={theme.colors['ink.background-primary']} />}


Noticed this color bug while i was exploring icons. It was black on black rather than black bg and white icon color.

camerow · 2024-11-25T22:44:59Z

apps/mobile/src/components/widgets/tokens/token-icon.tsx

@@ -7,7 +12,8 @@ interface TokenIconProps {
 export function TokenIcon({ ticker }: TokenIconProps) {
  switch (ticker) {
    case 'STX':
-      return <StxAvatarIcon />;
+      return <IconWithActivity avatar={<StxAvatarIcon />} activity="error" />;


TODO: Revert this change, just added for quick testing.

camerow · 2024-11-25T22:45:18Z

packages/ui/package.json

@@ -7,7 +7,7 @@
    "build": "pnpm panda && pnpm build:native && pnpm build:web",
    "build-storybook:web": "storybook build -c ./src/.storybook-web",
    "build:native": "tsup --config tsup.config.native.ts",
-    "build:watch": "concurrently  \"pnpm panda:watch\"  \"pnpm build:native --watch --preserveWatchOutput\"  \"pnpm build:web --watch\" ",
+    "build:watch": "concurrently  \"pnpm panda --watch\"  \"pnpm build:native --watch\"  \"pnpm build:web --watch\" ",


Incorrect syntax here, small fix while I was building the ui package.

pete-watters · 2024-11-26T09:20:26Z

packages/ui/src/icons/icon/icon-with-activity.native.tsx

+  activity,
+}: {
+  avatar: React.JSX.Element;
+  activity?: ActivityIconType;


If there is no activity maybe we should not add anything instead of defaulting to send?

Agree here, why would we default to send?

Still a draft, not intentionally left to be that way. I'll update to omit the icon if not provided as a optional prop.

pete-watters · 2024-11-26T09:26:26Z

packages/ui/src/icons/icon/icon-with-activity.native.tsx

@@ -0,0 +1,19 @@
+import { View } from 'react-native';


Sometimes we need to show an activity type icon on top of a protocol item, and sometimes we need to show a protocol icon on top for example here:

NOTE the design is wrong for the first two items - Stacks + Btc.

For subsequent tokens like sBTC, ALEX Lab etc. we need to show a base protocol icon on top of their specific item.

Maybe we can extend this component to be more broad and handle both activity and protocol sub-icons

Yeah that sounds good, should be somewhat easy to achieve.

To that point I notice we are hard coding our protocol icons - I would love to see us use something more generic like a <ProtocolIcon symbol="btc | stx | ..." />

pete-watters

Nice work so far here @camerow , I added a couple of suggestions but it's looking good overall

packages/ui/src/icons/activity-icons/activity-icon.native.tsx

fbwoolf · 2024-11-26T13:57:06Z

packages/ui/src/icons/arrow-down-icon.native.tsx

+export const ArrowDownIcon = forwardRef<Component, IconProps>(({ variant, ...props }, ref) => {
+  return (
+    <Icon ref={ref} {...props}>
+      {variants[variant ?? 'default']}


Is it nec to make this change when all other icons use a different code pattern?

No, was just playing with patterns and thinking about suggesting a different way of doing this down the line.

Actually as I was looking at this I was wondering why we don't just have a single component that can render any icon, eg <LeatherIcon icon="icon-name" variant="small" {...props} /> - I find that pattern makes adding icons much easier as you just have a lookup table / function and all new icons can just be added there.

Was the idea with this implementation that it allows for better tree-shaking?

I can revert it to how the others do it for now.

eg FontAwesome

Most of our components are based off Radix. https://www.radix-ui.com/icons

Was thinking on this, might be best to get @tigranpetrossian opinion here bc he is mostly owning the UI library? I don't have a strong opinion if others prefer a diff setup. It would just be quite a chunk of work to refactor as they are currently used this way in both web and mobile.

I think Will's proposed API will be quite an improvement. Radix has a different approach because it's a public set, and developers might only need a handful of icons. We on the other hand own our icon set, so having all the options under your fingertips in an autocomplete box is probably going to be much a better DX.
I'm going to add and track this under the umbrella issue for icon refactoring, some of which I added based on this PR. Within this PR it's best to keep icons as is though, just for consistency and to make the refactoring easier.

Yep, sounds good to me. 👍

fbwoolf · 2024-11-26T14:00:11Z

packages/ui/src/icons/icon/icon.native.tsx

@@ -16,7 +16,7 @@ export const Icon = forwardRef<Component, IconProps>(({ children, ...props }, re
  return (
    <>
      {React.cloneElement(child, {
-        color: theme.colors['ink.action-primary-default'],
+        color: props.color ?? theme.colors['ink.action-primary-default'],


Is this change nec? We have always been able to override the icon color w/out this. This should just set the default color?

It does make the fallback color more explicit and thus easier to debug.

Of course it can always be overriden without this, but the underlying svg usually has something like stroke="currentColor" and it's not obvious to me what that default is - having something explicitly set rather than a side effect seems like an improvement.

A bit of research says:

In React Native SVG, currentColor behaves similarly, but it inherits the color property from the nearest ancestor element.

Which isn't a desirable behavior - some sane default makes sense to me.

I would have to find the resource I used when implementing this with react-native-svg-transformer. I know it was the suggested way to set the svg currentColor using a theme color. I'm not totally following your explanation tbh.

Is this change nec? We have always been able to override the icon color w/out this. This should just set the default color?

I think I misunderstood your original comment - I thought it was regarding theme.colors['ink.action-primary-default'] which had already been there, you're right this isn't necessary given that the props are spread as the final argument.

tigranpetrossian · 2024-11-26T03:34:04Z

packages/ui/src/icons/plus-icon.native.tsx

 export const PlusIcon = forwardRef<Component, IconProps>(({ variant, ...props }, ref) => {
-  if (variant === 'small')


👍 All our icons follow this pattern, it's definitely time to re-visit, but ideally not one by one.
This can be rolled back for now. I'm putting together a list of improvements in Linear that will include this.

tigranpetrossian · 2024-11-26T03:34:58Z

packages/ui/src/icons/icon/icon.native.tsx

@@ -16,7 +16,7 @@ export const Icon = forwardRef<Component, IconProps>(({ children, ...props }, re
  return (
    <>
      {React.cloneElement(child, {
-        color: theme.colors['ink.action-primary-default'],
+        color: props.color ?? theme.colors['ink.action-primary-default'],


👍 Created an issue to add theme-aware color support for icons.

tigranpetrossian · 2024-11-26T03:43:51Z

packages/ui/src/icons/activity-icons/activity-icon.native.tsx

+    <View
+      style={{
+        position: 'absolute',
+        bottom: -4,
+        right: -6,
+        height: 24,
+        width: 24,
+        backgroundColor: theme.colors['ink.text-primary'],
+        borderColor: theme.colors['ink.background-primary'],
+        borderWidth: 3,
+        borderRadius: 1000,
+        justifyContent: 'center',
+        alignItems: 'center',
+      }}
+    >


Suggested change

<View

style={{

position: 'absolute',

bottom: -4,

right: -6,

height: 24,

width: 24,

backgroundColor: theme.colors['ink.text-primary'],

borderColor: theme.colors['ink.background-primary'],

borderWidth: 3,

borderRadius: 1000,

justifyContent: 'center',

alignItems: 'center',

}}

>

<Box

position="absolute"

bottom={-4}

right={-6}

justifyContent="center"

alignItems="center"

width={24}

height={24}

bg="ink.action-primary-default"

borderColor="ink.background-primary"

borderWidth={3}

borderRadius="round"

>

tigranpetrossian · 2024-11-26T03:47:29Z

packages/ui/src/icons/icon/icon-with-activity.native.tsx

In near future it's worth moving this logic over to Avatar, as I think the intention is to use secondary icons on other avatars for list items too. Will require refactoring both the web and native versions.

tigranpetrossian · 2024-11-27T15:48:33Z

packages/ui/src/icons/arrow-down-icon.native.tsx

+export const ArrowDownIcon = forwardRef<Component, IconProps>(({ variant, ...props }, ref) => {
+  return (
+    <Icon ref={ref} {...props}>
+      {variants[variant ?? 'default']}


I think Will's proposed API will be quite an improvement. Radix has a different approach because it's a public set, and developers might only need a handful of icons. We on the other hand own our icon set, so having all the options under your fingertips in an autocomplete box is probably going to be much a better DX.
I'm going to add and track this under the umbrella issue for icon refactoring, some of which I added based on this PR. Within this PR it's best to keep icons as is though, just for consistency and to make the refactoring easier.

feat(ui): icon with activity sub icon

7729e28

camerow requested review from fbwoolf, pete-watters and tigranpetrossian November 25, 2024 22:37

camerow commented Nov 25, 2024

View reviewed changes

pete-watters reviewed Nov 26, 2024

View reviewed changes

pete-watters approved these changes Nov 26, 2024

View reviewed changes

fbwoolf reviewed Nov 26, 2024

View reviewed changes

packages/ui/src/icons/activity-icons/activity-icon.native.tsx Outdated Show resolved Hide resolved

fbwoolf reviewed Nov 26, 2024

View reviewed changes

camerow added 3 commits November 26, 2024 12:16

feat: add color and update prop name

875adf7

chore: use existing pattern

0bfdb04

chore: omit activity icon when not provided

213b23b

camerow force-pushed the ui/asset-icon-with-activity branch from 4a8e6ab to 213b23b Compare November 26, 2024 20:32

tigranpetrossian approved these changes Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Icon with activity sub-icon #654

Draft: Icon with activity sub-icon #654

camerow commented Nov 25, 2024 •

edited

Loading

camerow Nov 25, 2024 •

edited

Loading

camerow Nov 25, 2024

camerow Nov 25, 2024

pete-watters Nov 26, 2024

fbwoolf Nov 26, 2024

camerow Nov 26, 2024

pete-watters Nov 26, 2024

camerow Nov 26, 2024

pete-watters left a comment

fbwoolf Nov 26, 2024

camerow Nov 26, 2024

camerow Nov 26, 2024

fbwoolf Nov 27, 2024

fbwoolf Nov 27, 2024

tigranpetrossian Nov 27, 2024

fbwoolf Dec 2, 2024

fbwoolf Nov 26, 2024

camerow Nov 26, 2024

camerow Nov 26, 2024

fbwoolf Nov 27, 2024 •

edited

Loading

camerow Nov 27, 2024

tigranpetrossian Nov 26, 2024

tigranpetrossian Nov 26, 2024

tigranpetrossian Nov 26, 2024

tigranpetrossian Nov 26, 2024

tigranpetrossian Nov 27, 2024

		export const PlusIcon = forwardRef<Component, IconProps>(({ variant, ...props }, ref) => {
		if (variant === 'small')

Draft: Icon with activity sub-icon #654

Are you sure you want to change the base?

Draft: Icon with activity sub-icon #654

Conversation

camerow commented Nov 25, 2024 • edited Loading

camerow Nov 25, 2024 • 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

pete-watters 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbwoolf Nov 27, 2024 • 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

camerow commented Nov 25, 2024 •

edited

Loading

camerow Nov 25, 2024 •

edited

Loading

fbwoolf Nov 27, 2024 •

edited

Loading