feat: approver pattern #5027

kyranjamie · 2024-03-05T18:39:57Z

Building Leather at commit 90b1724

This PR adds the <Approver /> component as described in Notion.

Initial demo

2024-03-05-000124.mp4

Todo

Add tooltip to Header component
Figure out best way to handle success screens (maybe just a duplicate Approver)
Add friction to Approve button cc/ @fabric-8
Blocked: header is outside the container currently when window's resized, not sure if this is easy to fix with Pete's work?

pete-watters · 2024-03-06T05:47:05Z

src/app/features/approver/components/approver-actions.tsx

@@ -0,0 +1,26 @@
+import { css } from 'leather-styles/css';


Just a heads up, I meant to mention before, I have a <Footer in the containers branch we can eventually use instead of this

Will that play nicely with the fixed positioning though?

The footers are all supposed to be sticky now. Ignore my Footer for now and go as planned and then we can see if we can refactor them to share.

They are also supposed to have a top border when the content above them is scrollable but I didn't implement that yet

pete-watters · 2024-03-06T05:48:50Z

src/app/features/approver/approver.stories.tsx

+  title: 'Feature/Approver',
+
+  render: ({ children, ...args }) => (
+    <Flex maxW="390px" h="680px" border="1px solid lightgrey" overflowY="auto">


Mark has told me to make the popup height 600px. Is that good with you? Or do you think we should have it variable based on the popup type?

I found that popup.ts accepts dimensions but we don't use them

src/app/common/has-children.ts

pete-watters · 2024-03-06T05:51:44Z

src/app/common/hooks/use-element-height-listener.ts

@@ -0,0 +1,22 @@
+import { useEffect } from 'react';


This seems cool and maybe useful to me for dynamic height sizing which has proved tricky in containers.

I was trying to use calculated heights via variables which panda doesn't like and had considered using style so this is a good idea

pete-watters · 2024-03-06T05:52:42Z

The Header issue you mentioned is resolved in my work so you should be OK 👍

alter-eggo

💪 👍

alter-eggo · 2024-03-06T08:09:32Z

package.json

@@ -197,6 +197,7 @@
    "ecdsa-sig-formatter": "1.0.11",
    "ecpair": "2.1.0",
    "formik": "2.4.5",
+    "framer-motion": "11.0.8",


src/app/common/hooks/use-register-children.ts

alter-eggo · 2024-03-06T08:22:56Z

src/app/features/approver/components/approver-advanced.tsx

+const slightPauseForContentEnterAnimation = createDelay(120);
+
+export function ApproverAdvanced({ children }: HasChildren) {
+  const { isDisplayingAdvancedView, setIsDisplayingAdvancedView } = useApproverContext();


can there be multiple advanced views? if so it seems this will close/open all of them at once?

It isn't built to support this, and don't think we will @mica000 ?

Maybe I proactively reject two Approver components?

Correct, only one advanced view

alter-eggo · 2024-03-06T08:47:47Z

src/app/features/approver/approver.stories.tsx

+              <Button variant="outline">Cancel</Button>
+              <Button>Approve</Button>


thinking in most cases there will be just these cancel/approve btns no? maybe create them as default in Approver.Actions?
but this means we need to pass approve/cancel actions as props, so quite arguable

This is a tough one. I separated the actions (as prop) from the children, so we apply the button styles implicitly (flex with stretch) but adding the buttons themselves I'm not sure, maybe in some cases we want different ones?

Not sure how to handle exactly. Esp. if we have the animated confirm btn.

In the case that we have behaviour such as in latest designs, e.g.

think it would need to be part of approver to share the state

mica000 · 2024-03-07T17:20:23Z

This is amazing!!!! 😍😍😍😍😍😍

I noticed we are using 12 + 24px in the margins and 12px between ItemLayout and heading:

This is happening because we don't have the heading component in development, so the structure will naturally be different. Wondering what we could do to preserve the same values. Maybe adding 24px between heading and itemLayout and 24px around all the margin will solve it?

Image of how it is in designs:

Also, the heading has a placeholder icon to show a tooltip that is sometimes present. Curious on how we would handle that as well.

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

kyranjamie · 2024-03-08T09:54:10Z

Thanks Michelly.

I haven't made sure the margins are accurate yet, so good you point it out. think it might be best to put a min-height on the header element. Will come up with a god approach today and show you.

The margins should look visually identical to designs, but be aware of the changes made in these PRs #5014 #5024.

In Figma you've padded the ItemInteractive for the hover state, but if we build it like that then it gets complicated because we need to use "compound margins" on the container (12 + 12) and every other element to get the right spacing. See storybook for demo of this. Now we use pesudo elements for the background so the core element needs to adjustment and aligns by default.

pete-watters reviewed Mar 6, 2024

View reviewed changes

src/app/common/has-children.ts Show resolved Hide resolved

pete-watters reviewed Mar 6, 2024

View reviewed changes

alter-eggo reviewed Mar 6, 2024

View reviewed changes

kyranjamie force-pushed the feat/approver branch 5 times, most recently from cb142fc to 12bcc8c Compare March 7, 2024 16:54

kyranjamie force-pushed the feat/approver branch 2 times, most recently from a546d71 to 9a5a12d Compare March 18, 2024 09:23

kyranjamie force-pushed the feat/approver branch 4 times, most recently from 7b42ae5 to d54ded8 Compare April 1, 2024 15:44

kyranjamie added 7 commits April 9, 2024 09:30

feat: approver pattern component

941e6a5

fix: add animations

bf0903c

fix: track approver children

a5a7c56

fix: updated icons

e5dea01

chore: tooltip in header

b4b97f7

feat: animated status line

a816c0b

temp: lksjdf

90b1724

kyranjamie force-pushed the feat/approver branch from d54ded8 to 90b1724 Compare April 9, 2024 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: approver pattern #5027

feat: approver pattern #5027

kyranjamie commented Mar 5, 2024 •

edited by leather-bot

pete-watters Mar 6, 2024

kyranjamie Mar 6, 2024

pete-watters Mar 6, 2024 •

edited

pete-watters Mar 6, 2024

pete-watters Mar 6, 2024

pete-watters commented Mar 6, 2024

alter-eggo left a comment

alter-eggo Mar 6, 2024

alter-eggo Mar 6, 2024

kyranjamie Mar 6, 2024

kyranjamie Mar 6, 2024

mica000 Mar 6, 2024

alter-eggo Mar 6, 2024 •

edited

kyranjamie Mar 6, 2024

kyranjamie Apr 5, 2024

mica000 commented Mar 7, 2024 •

edited

kyranjamie commented Mar 8, 2024

		<Button variant="outline">Cancel</Button>
		<Button>Approve</Button>

feat: approver pattern #5027

Are you sure you want to change the base?

feat: approver pattern #5027

Conversation

kyranjamie commented Mar 5, 2024 • edited by leather-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters commented Mar 6, 2024

alter-eggo 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

alter-eggo Mar 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mica000 commented Mar 7, 2024 • edited

This is amazing!!!! 😍😍😍😍😍😍

kyranjamie commented Mar 8, 2024

kyranjamie commented Mar 5, 2024 •

edited by leather-bot

pete-watters Mar 6, 2024 •

edited

alter-eggo Mar 6, 2024 •

edited

mica000 commented Mar 7, 2024 •

edited