refactor(send-form): remove custom dropdown type #4975

kyranjamie · 2024-02-23T12:40:12Z

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

Making a separate PR so changes are easier to review. Related to #4960.

This PR removes the custom dropdown we had implemented for the recipient type dropdown: address or bns name. This simplifies our code in removing a duplicate component, and using a legit dropdown component. @fbwoolf knows how thrilled I will be to remove it haha.

Unable to use custom trigger
Our Dropdown component assumes we'll always use the same trigger. In this instance we need a custom trigger. Temp solution here is to export an Unstyled trigger and use that instead. But curious to hear ideas how we might do better than this? Should we never assume the trigger styles and always pass it in?

From the looks of the designs, the normal trigger is just a button with an arrow in it. Should we could make a separate <DropdownButton /> component and pass that into <Dropdown.Trigger />? Leaving Trigger unstyled in the core dropdown? Pretty sure we'll need custom trigger dropdowns again in future.

Added reverse prop to <Flag />
Per definition of Flag object, it can have a reverse prop, which I wanted to utilise for this bit of UI, so I just added it.

pete-watters · 2024-02-23T13:24:57Z

src/app/pages/send/send-crypto-asset-form/family/bitcoin/components/bitcoin-recipient-field.tsx

@@ -1,7 +1,7 @@
 import { fetchBtcNameOwner } from '@app/query/stacks/bns/bns.utils';

-import { GenericRecipientField } from '../../../components/recipient-fields/generic-recipient-field';
+import { RecipientField } from '../../../components/recipient-fields/recipient-field';


It'd be nice to refactor the path while we're here ../../../

pete-watters

This LGTM. It might be nice to wrap the Dropdown and then pass the trigger in a slot prop

fbwoolf · 2024-02-23T14:37:12Z

@kyranjamie I think commits are included here that you didn't intend to be reviewed again? Will just review the dropdown changes here.

kyranjamie · 2024-02-23T14:37:58Z

Yep, fixing now

src/app/ui/components/flag/flag.tsx

fbwoolf · 2024-02-23T14:43:09Z

src/app/ui/components/flag/flag.tsx

@@ -33,13 +34,23 @@ interface FlagProps extends FlexProps {
 export function Flag({
  spacing = 'space.03',
  align = 'middle',
+  reverse = false,


This will be helpful, was thinking about this yesterday with the Callout.

fbwoolf

It prob does make sense to make the trigger customizable. 👍

kyranjamie · 2024-02-23T14:51:17Z

It prob does make sense to make the trigger customizable. 👍

see 3b57aa2 and LMK what you think. Now thinking it should be a Button instead with some custom styles?

alter-eggo

lgtm 👍
re: unstyled trigger, I'm ok with the current solution, just think we can put this trigger you created to a "DefaultDropdownTrigger" variable or smth like that. so we don't copy paste it in future?

also think it would be nice to add rotate animation there

...s/send/send-crypto-asset-form/components/recipient-type-dropdown/recipient-type-dropdown.tsx

fbwoolf · 2024-02-23T15:13:43Z

see 3b57aa2 and LMK what you think. Now thinking it should be a Button instead with some custom styles?

If that ends up working better, def fine with it.

kyranjamie · 2024-02-23T16:39:35Z

I've made some updates to the dropdown animations:

Before

2024-02-23-000109.mp4

After

2024-02-23-000111.mp4

Improvements

Fixed exit animation: Previously, the dropdown would only animate in, and on closing disappear instantly.
Added user-select: none: Prevents highlighting of text which often happens when clicking on button, seen in before video
Rotating chevron on open/close: Per @alter-eggo suggestion. It's an improvement, but not in love with it. @fabric-8 challenge you to come up with something fun ⚔️ Too much, killed
Removed custom bezier curve: @fbwoolf not sure where this came from but a simple fade in up/out down with ease-in-out works okay. Open to adding back other easing suggestions.
Remove min-width: Cc @mica000 to confirm. Previously we had min width 256px, but that is pretty big when used for the tiny dropdown.
Removed focus state: Bad for accessibility but was super ugly with default outline. Open to adding back something better.

Before	After

fbwoolf · 2024-02-23T17:12:39Z

@kyranjamie your improvements lgtm, I think I just wasn't sure what we wanted or where it would be applied when implemented ...so, I believe I used the primitive example properties from Radix dropdown menu for the animations.

fbwoolf · 2024-02-23T17:15:40Z

In regards to the focused state, we do seem to have a really bold focused state everywhere, I wonder if that should be less emphasized overall and NOT a bright blue? It was in the designs for the ItemInteractive too.

fabric-8 · 2024-02-26T11:17:28Z

Nice! I'd recommend to remove the open/close animation for the chevron here, it's too much. Fine to just leave it stay static, sometimes, less is more — How about adding a hover state using component-background-hover?

mica000 · 2024-02-26T11:21:12Z

@kyranjamie awesome improvements!! love it!

Wondering if updating to this would make sense? If so, let me know as we need to review the design with the team.

https://www.figma.com/file/FlmF7ropcijOb7iVSDqNn0/%F0%9F%9F%A7-Web-App-Explorations-2024-H1?type=design&node-id=1214-134266&mode=design&t=vjpn8FdOAx69G198-4

kyranjamie · 2024-02-27T14:04:29Z

I will make a new issue for this work @mica000 so we can merge this one as is

fbwoolf · 2024-02-27T21:31:19Z

src/app/ui/components/dowpdown-menu/dropdown-menu.tsx

+  userSelect: 'none',
+  '[data-state=open] &': { bg: 'ink.component-background-pressed' },
+});
+function Button({ children, ...props }: HTMLStyledProps<'div'>) {


@kyranjamie rebased on these changes and slightly confused bc this doesn't appear to be used? You coming back to it?

Nvm, I see what is happening here, you used the 'unstyled' trigger in code and left this for use as the styled button. I am working on fixing the settings menu trigger button in Pete's work, so in this code a bit.

kyranjamie requested review from pete-watters, fbwoolf, alter-eggo and edgarkhanzadian February 23, 2024 12:40

kyranjamie force-pushed the fix/memo branch from 3384666 to cc4de7e Compare February 23, 2024 13:24

pete-watters reviewed Feb 23, 2024

View reviewed changes

pete-watters approved these changes Feb 23, 2024

View reviewed changes

kyranjamie force-pushed the fix/memo branch from cc4de7e to 87ac735 Compare February 23, 2024 13:41

kyranjamie changed the base branch from fix/memo to dev February 23, 2024 14:28

fbwoolf reviewed Feb 23, 2024

View reviewed changes

src/app/ui/components/flag/flag.tsx Outdated Show resolved Hide resolved

fbwoolf reviewed Feb 23, 2024

View reviewed changes

fbwoolf approved these changes Feb 23, 2024

View reviewed changes

kyranjamie force-pushed the refactor/recipient-address-type-dropdown branch from 361ff1d to d17861e Compare February 23, 2024 14:50

kyranjamie force-pushed the refactor/recipient-address-type-dropdown branch from d17861e to 3b57aa2 Compare February 23, 2024 14:51

alter-eggo approved these changes Feb 23, 2024

View reviewed changes

...s/send/send-crypto-asset-form/components/recipient-type-dropdown/recipient-type-dropdown.tsx Outdated Show resolved Hide resolved

kyranjamie force-pushed the refactor/recipient-address-type-dropdown branch from 9e0ed71 to e9f211e Compare February 23, 2024 16:27

kyranjamie force-pushed the refactor/recipient-address-type-dropdown branch from e9f211e to 1c28777 Compare February 23, 2024 17:06

kyranjamie force-pushed the refactor/recipient-address-type-dropdown branch from 1c28777 to b35e5c0 Compare February 27, 2024 14:03

kyranjamie added 2 commits February 27, 2024 15:04

refactor(send-form): remove custom dropdown type

a61f93c

refactor(dropdown): unstyled trigger, separate button

afd9637

refactor(dropdown): update animations

604e0b4

kyranjamie force-pushed the refactor/recipient-address-type-dropdown branch from b35e5c0 to 604e0b4 Compare February 27, 2024 14:04

kyranjamie mentioned this pull request Feb 27, 2024

UI: update recipient field input #4989

Open

kyranjamie enabled auto-merge February 27, 2024 14:05

kyranjamie added this pull request to the merge queue Feb 27, 2024

Merged via the queue into dev with commit 6656e05 Feb 27, 2024
28 checks passed

kyranjamie deleted the refactor/recipient-address-type-dropdown branch February 27, 2024 14:37

fbwoolf reviewed Feb 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(send-form): remove custom dropdown type #4975

refactor(send-form): remove custom dropdown type #4975

kyranjamie commented Feb 23, 2024 •

edited by github-actions bot

Loading

pete-watters Feb 23, 2024

pete-watters left a comment

fbwoolf commented Feb 23, 2024

kyranjamie commented Feb 23, 2024

fbwoolf Feb 23, 2024

fbwoolf left a comment

kyranjamie commented Feb 23, 2024 •

edited

Loading

alter-eggo left a comment •

edited

Loading

fbwoolf commented Feb 23, 2024

kyranjamie commented Feb 23, 2024 •

edited

Loading

fbwoolf commented Feb 23, 2024

fbwoolf commented Feb 23, 2024

fabric-8 commented Feb 26, 2024

mica000 commented Feb 26, 2024 •

edited

Loading

kyranjamie commented Feb 27, 2024

fbwoolf Feb 27, 2024

fbwoolf Feb 27, 2024

refactor(send-form): remove custom dropdown type #4975

refactor(send-form): remove custom dropdown type #4975

Conversation

kyranjamie commented Feb 23, 2024 • edited by github-actions bot Loading

pete-watters Feb 23, 2024

Choose a reason for hiding this comment

pete-watters left a comment

Choose a reason for hiding this comment

fbwoolf commented Feb 23, 2024

kyranjamie commented Feb 23, 2024

fbwoolf Feb 23, 2024

Choose a reason for hiding this comment

fbwoolf left a comment

Choose a reason for hiding this comment

kyranjamie commented Feb 23, 2024 • edited Loading

alter-eggo left a comment • edited Loading

Choose a reason for hiding this comment

fbwoolf commented Feb 23, 2024

kyranjamie commented Feb 23, 2024 • edited Loading

fbwoolf commented Feb 23, 2024

fbwoolf commented Feb 23, 2024

fabric-8 commented Feb 26, 2024

mica000 commented Feb 26, 2024 • edited Loading

kyranjamie commented Feb 27, 2024

fbwoolf Feb 27, 2024

Choose a reason for hiding this comment

fbwoolf Feb 27, 2024

Choose a reason for hiding this comment

kyranjamie commented Feb 23, 2024 •

edited by github-actions bot

Loading

kyranjamie commented Feb 23, 2024 •

edited

Loading

alter-eggo left a comment •

edited

Loading

kyranjamie commented Feb 23, 2024 •

edited

Loading

mica000 commented Feb 26, 2024 •

edited

Loading