feat: ui items #4881

fbwoolf · 2024-01-30T16:50:14Z

Try out this version of Leather — Extension build, Test report

This PR builds/shares a reusable Item component.

Assets

Activity

Accounts

Send

Receive

Swap

fbwoolf · 2024-01-30T16:55:02Z

@mica000 @fabric-8 use the Storybook link to check over the Item component here when you can.

src/app/features/leather-intro-dialog/leather-intro-steps.tsx

src/app/ui/components/items/item-default.layout.tsx

src/app/ui/components/items/item.stories.tsx

src/app/ui/components/items/item.tsx

theme/recipes/button-recipe.ts

pete-watters

Great work here Fara! 👏 🚀

mica000 · 2024-01-31T13:39:39Z

This is great @fbwoolf! Thank you so much!
Was looking very much forward for this component as it will be used in many places!

On Storybooks the item seems broken, so not sure what is happening there?
In relation to the "arrow right", that appears on send, account and activity, let's use the variant without the arrow. For now we are only using the "arrow right" on the Approval UX redesign views as a way to move the user to a page such as Account or Fee.
Corner radius: 2px on the item
Token and status: Will create a ticket to handle token and status

kyranjamie

Great work Fara.

Please forgive the many questions on how this works, and some of the decisions. Also curious if we're going to use this with Radix primitive components as well.

kyranjamie · 2024-01-31T13:43:34Z

src/app/ui/components/items/item.tsx

+      bg: 'accent.background-primary',
+      height: 'auto',
+      userSelect: 'none',
+      p: 'space.03',


I wonder if padding should not be defined here? @fabric-8 @mica000 are we absolutely sure we only ever use one padding unit inside them?

Yeah for now that what we do indeed, so far we didn't run into cases where we'd need to go differently about that 🤷

src/app/ui/components/items/item-with-buttons.layout.tsx

src/app/components/account/account-list-item-layout.tsx

src/app/ui/components/items/item-default.layout.tsx

src/app/ui/components/items/item.stories.tsx

src/app/ui/components/items/item.tsx

src/app/pages/receive/components/receive-item.tsx

kyranjamie · 2024-01-31T14:06:12Z

src/app/ui/components/items/item.tsx

+  function onKeyDown(e: KeyboardEvent<HTMLInputElement>) {
+    if (!isRoleButton) return;
+    if (e.key === 'enter' || e.key === ' ') {
+      e.preventDefault();
+      onClick();
+    }
+  }


Why is this needed? If the element's a button it should behave this way by default.

I'm always cautious of code which tries to replicate how browser elements. There's always way more to them than you think. Reminds me of https://remysharp.com/2016/12/11/how-tabs-should-work as an example. If there's a way we can just use the browser behaviour than recreating, that's preferable.

This is not always a button. I am adding this here after referencing how to add accessibility to a div so that it can be either a div or a button.

fbwoolf · 2024-01-31T14:34:21Z

@mica000 why do you say the storybook items are broken? 🤔 https://65982789c7e2278518f189e7-vbbtozlkvb.chromatic.com/?path=/story/item--default ...they seem fine to me?

mica000 · 2024-01-31T14:43:22Z

@mica000 why do you say the storybook items are broken? 🤔 https://65982789c7e2278518f189e7-vbbtozlkvb.chromatic.com/?path=/story/item--default ...they seem fine to me?

I see them like this:

Seems like the padding is off?
Here is how it is in desgins:

fbwoolf · 2024-01-31T15:36:33Z

@mica000 why do you say the storybook items are broken? 🤔 https://65982789c7e2278518f189e7-vbbtozlkvb.chromatic.com/?path=/story/item--default ...they seem fine to me?

Strange, not what I see ...will double check code bc I'm not used to working with Grids.

fabric-8 · 2024-01-31T21:04:13Z

It look fine to me a bit earlier but now I'm also getting this:

Wanted to call out that we changed the height of text elements to 20px after removing the link elements in figma, gonna look at it later when it's maybe rendered correctly again :)

fbwoolf · 2024-02-02T01:05:35Z

~~Moving this into draft mode until I'm done refactoring.~~

fbwoolf · 2024-02-02T02:27:35Z

@kyranjamie let me know what you think of this version in your AM if you have time, thx.

EDIT: There is likely more refactoring we can do to simplify the sharing of the asset list items, but this might be a good step until we have new designs. Also, quite a few components typically pass components as props, so I used isValidElement to render the passed jsx. Styling isn't perfect with the interaction yet, but wanted to get feedback on this direction.

kyranjamie

Thanks for trying some different solutions @fbwoolf

kyranjamie · 2024-02-02T10:29:50Z

src/app/components/account/account-list-item.layout.tsx

+import { ItemLayout } from '@app/ui/components/item/item.layout';
+import { Spinner } from '@app/ui/components/spinner';
+
+interface AccountListItemLayoutProps extends ComponentProps<'div'> {


We don't spread the additional props, so either remove type, or spread props

kyranjamie · 2024-02-02T10:34:30Z

src/app/ui/components/item/item.tsx

+interface RootProps {
+  isDisabled?: boolean;
+  isInteractive?: boolean;
+  onClickItem?(): void;
+}
+function Root({ children, isDisabled, isInteractive, onClickItem }: RootProps & HasChildren) {
+  return isInteractive ? (
+    <ItemInteractive disabled={isDisabled} onClick={onClickItem}>
+      {children}
+    </ItemInteractive>
+  ) : (
+    <ItemBase>{children}</ItemBase>
+  );
+}


There's no value in using a slot component if we just had a single one. Recommend changing to a single component.

On isInteractive, this could just be inferred from whether there's a onClick defined or not? The redundancy creates the possibility for illogical props isInteractive={false} onClick={() => {...}}

I reckon that, as the behaviours of ItemInteractive and ItemBase are different, they don't belong tied together in a single component. It's only because of past use of isPressable that they are in this solution. If this is too complex to remove, I think it's okay to leave as this, I just question whether this ternary logic should belong in the component itself.

For instance, what if we were to remove isPressable, and in all the places where it's used we just use ItemInteractive and disable it if there isn't an action? That would make more sense I think?

I'm not using a slot component, but did use the naming of Root so that is prob confusing. I am just using a config recipe, not a slot recipe in this refactor.

I am using isDefined(onClick) in the component to set the isInteractive prop, but I can move to handle it in the item, np ...just thought it was more explicit with our use atm.

We can't really remove the isPressable situation w/out a bigger refactor here, and I don't think we should take the time to do it rn with new designs coming anyway for most of these features? I thought about, and tried, just disabling the ItemInteractive when not pressable, but a disabled state/styling is really different than this. That is why I chose to use the shared base styling and applying the variant of interactive to a different component. Also, we do get the console error of a button in a button in some cases (with the Connect Ledger button) when it is embedded in the asset list.

src/app/features/leather-intro-dialog/leather-intro-steps.tsx

kyranjamie · 2024-02-02T10:40:49Z

src/app/ui/components/item/item.tsx

+
+const ItemInteractive = forwardRef<HTMLButtonElement, HTMLStyledProps<'button'> & ItemVariantProps>(
+  (props, ref) => (
+    <styled.button className={`group ${itemRecipe({ interactive: true })}`} ref={ref} {...props} />


What's the group class about?

group selectors in panda are pretty cool. This is how I'm handling the isDisabled state of children. You then apply groupDisabled to the children and they will inherit that state.

https://panda-css.com/docs/concepts/conditional-styles#group-selectors

kyranjamie · 2024-02-02T11:18:16Z

The disabled state is the more complex requirement here. We need a way to communicate the state of the button to child content.

A component API that optionally allows a render component would work well I think.

<InteractiveItem>Simple use-case</InteractiveItem>

<InteractiveItem isDisabled>
  {({ isDisabled }) => (<ItemContent isDisabled={isDisabled} /> )}
</InteractiveItem>

That, or we just duplicate the isDisabled prop? Wonder if necessary.

How/where is this handled in the current implementation?

kyranjamie · 2024-02-02T11:33:41Z

As far of this effort, I wonder if you can look into how we could best apply these same InteractiveItem styles to the dropdown item

Figma link https://www.figma.com/file/2MLHeIeL6XPVi3Tc2DfFCr/%E2%9D%96-Leather-%E2%80%93-Design-System?type=design&node-id=8945-105536&mode=design&t=1tV8WbAOcIewYOVX-4

Practically all the styles are shared

mica000 · 2024-02-02T12:24:21Z

@fbwoolf @kyranjamie Designs updated here: https://www.figma.com/file/2MLHeIeL6XPVi3Tc2DfFCr/%E2%9D%96-Leather-%E2%80%93-Design-System?type=design&node-id=2081-3746&mode=design

fbwoolf · 2024-02-02T14:27:13Z

The disabled state is the more complex requirement here. We need a way to communicate the state of the button to child content.

How/where is this handled in the current implementation?

There is a isDisabled variant being used. Ref the deployed storybook and look at the stories I built. The group selector is handling changing the styles of children.

I actually don't think the render prop pattern is suggested anymore by React. The docs say it is deprecated for using custom hooks?

https://legacy.reactjs.org/docs/render-props.html

fbwoolf · 2024-02-02T14:29:17Z

As far of this effort, I wonder if you can look into how we could best apply these same InteractiveItem styles to the dropdown item

Practically all the styles are shared

After working on these, I'm not sure it is nec, is it? Thestyles are pretty simple.

kyranjamie · 2024-02-02T14:40:53Z

There is a isDisabled variant being used. Ref the deployed storybook and look at the stories I built. The group selector is handling changing the styles of children.

Nice, didn't know this.

I actually don't think the render prop pattern is suggested anymore by React. The docs say it is deprecated for using custom hooks?

Because of group, no need to add the render prop here then. Though, I think we still have legit cases for render props, eg the loader pattern. We're using it exclusively because we can't do what we want with hooks (handling conditionality).

After working on these, I'm not sure it is nec, is it? Thestyles are pretty simple.

Would still be nice not to repeat them however simple, if they're exactly the same?

fbwoolf · 2024-02-02T14:51:12Z

Would still be nice not to repeat them however simple, if they're exactly the same?

Yeah, was looking at them again. Seems like a good idea to share the different state styling b/w them.

fbwoolf · 2024-02-03T02:32:16Z

@kyranjamie maybe look this over if get a sec, I was able to refactor to one ItemInteractive with this version and I'm sharing item styles with the Select and DropdownMenu items.

kyranjamie · 2024-02-03T19:31:00Z

@kyranjamie maybe look this over if get a sec, I was able to refactor to one ItemInteractive with this version and I'm sharing item styles with the Select and DropdownMenu items.

Yeah looks great Fara. Super tidy 💯

fbwoolf · 2024-02-03T22:53:05Z

Added the ItemLayout story here...

fbwoolf force-pushed the feat/ui-items branch from c2d9fc9 to db9c29d Compare January 30, 2024 16:51

This was linked to issues Jan 30, 2024

Design System: InteractiveItem #4314

Closed

Broken disabled button styles #4840

Closed

fbwoolf requested review from kyranjamie, alter-eggo, edgarkhanzadian, pete-watters, mica000 and fabric-8 January 30, 2024 16:53