Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: redesign connection flow #5874

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

feat: redesign connection flow #5874

wants to merge 1 commit into from
+1,537 −1,666

Conversation

kyranjamie
Copy link
Collaborator

@kyranjamie kyranjamie commented Sep 25, 2024 •
edited by leather-bot
Loading

Try out Leather build 6974dcd — Extension build, Test report, Storybook, Chromatic

Closes leather-io/issues#283

This PR implements the redesigned getAddresses flow. Using the Approver UX that's now in the design system. I've also added a warning in the inpage.ts to alert app devs they should upgrade.

Screenshots

Light Dark Switch account
image image image

Demo

2024-09-25-000296.mp4

@kyranjamie kyranjamie force-pushed the feat/redesign-get-addresses branch 2 times, most recently from 873180e to 59d6da9 Compare September 25, 2024 09:45
@pete-watters
Copy link
Contributor

Nice work 👍 LGTM on initial glance

fbwoolf
fbwoolf approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@fbwoolf fbwoolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm ...should the icon you are adding here be shared form the UI lib?

@kyranjamie
Copy link
Collaborator Author

should the icon you are adding here be shared form the UI lib?

It's not really an icon like the others, just an illustration

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 25, 2024 •
edited
Loading

It's not really an icon like the others, just an illustration

We do have an assets-web folder in the UI lib so that is confusing?

@kyranjamie
Copy link
Collaborator Author

What's the purpose of that folder exactly? For assets that belong in the ui library? Is it supposed to be that all image assets go there, even if they don't correspond to a given ui component?

@fbwoolf
Copy link
Contributor

fbwoolf commented Sep 25, 2024

What's the purpose of that folder exactly? For assets that belong in the ui library? Is it supposed to be that all image assets go there, even if they don't correspond to a given ui component?

Tbh, I don't know. @pete-watters I believe you created it? It was part of the build when I started working on icons.

@pete-watters
Copy link
Contributor

What's the purpose of that folder exactly? For assets that belong in the ui library? Is it supposed to be that all image assets go there, even if they don't correspond to a given ui component?

Tbh, I don't know. @pete-watters I believe you created it? It was part of the build when I started working on icons.

@edgarkhanzadian added it when first building the UI package. It's similar to public/assets in the extension, containing web only static assets .png images, font files etc.

There are both an icons and illustration folder there. I would put this asset in assets/icons along with the other .svg files if it was being shared with native but as it's just used on web, I think it's OK where it is now

alter-eggo
alter-eggo reviewed Sep 26, 2024
View reviewed changes
Copy link
Contributor

@alter-eggo alter-eggo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, great to see that animations 💪
I only feel like we can do them a little bit faster when show connection page?

@kyranjamie kyranjamie force-pushed the feat/redesign-get-addresses branch 2 times, most recently from 4a3d089 to f898af4 Compare September 27, 2024 16:00
@kyranjamie kyranjamie marked this pull request as ready for review September 27, 2024 16:01
@kyranjamie kyranjamie force-pushed the feat/redesign-get-addresses branch 2 times, most recently from 04aecd3 to 0653e3e Compare September 27, 2024 16:07
@kyranjamie
Copy link
Collaborator Author

I only feel like we can do them a little bit faster when show connection page?

Agree, we can tune up a tad in mono

@markmhendrickson
Copy link
Collaborator

Looking good! Should we integrate Crowdin here as a first extension-side integration, for the copy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabric-8 fabric-8 Awaiting requested review from fabric-8

@pete-watters pete-watters Awaiting requested review from pete-watters

@alter-eggo alter-eggo Awaiting requested review from alter-eggo

@fbwoolf fbwoolf Awaiting requested review from fbwoolf

@alexp3y alexp3y Awaiting requested review from alexp3y

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kyranjamie @pete-watters @fbwoolf @markmhendrickson @alter-eggo