-
Notifications
You must be signed in to change notification settings - Fork 15
feat(ui): [LW 9028] Multi wallet UI elements #728
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.
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
Conversation
Allure report
smokeTests: ✅ test report for 30389036
|
| export const WalletOption = ({ | ||
| id, | ||
| disabled, | ||
| open = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open is unused.
| }; | ||
| }; | ||
|
|
||
| export const WalletOption = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this components seem to be very specific for a single use case in Lace, maybe lets move it into the Lace app repository and setup Storybook there? And also it seems to be a near duplicate to the Trigger component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The items mentioned actually belong to the UI Toolkit. The UX team is currently updating the Navigation & Toolbars section to include the latest specifications.
From these specs, you can create a LinkOption or ToggleOption. It might be a good idea to rename WalletOption to ProfileDetailOption to make it more general. However, I personally believe that we should stick to something that conveys the true purpose, which is a wallet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it 👍 Yeah, i still think that a ui framework shouldn't suggest the final app component being built from framework parts. We had this discussion many times in relation to react-polymorph in Daedalus and that's why we kept app specific components which could not be generalized (or didn't have to be re-used) in Daedalus Storybook.
I'm OK with picking more general purpose names (LinkOption or ToggleOption) like you suggested. Otherwise it becomes confusing because we could start moving any ui component from Lace into @lace/ui which would defeat the purpose of creating a foundational ui library used by many apps (like Dex, DApps store etc. potential more in the future)
Probably we need to bring this up with the design team to be on the same page
|
@renanvalentin can you explain the difference between the |
bb14e88 to
a317df8
Compare
packages/ui/src/design-system/profile-dropdown/profile-dropdown-trigger.component.tsx
Outdated
Show resolved
Hide resolved
b1b7b7e to
bb9ef09
Compare
bb9ef09 to
cad43c4
Compare
|
@lucas-barros @DominikGuzei ready for review |
| import type { ComponentStory, Meta } from '@storybook/react'; | ||
| import { within, userEvent } from '@storybook/testing-library'; | ||
|
|
||
| import { ThemeColorScheme, ThemeProvider } from '../../design-tokens'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ThemeProvider and ThemeColorScheme are not used.
|
Kudos, SonarCloud Quality Gate passed!
|
DominikGuzei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work 🎉









Checklist
Proposed solution
This is a partial implementation of the personal dropdown from Lace UI's toolkit. Below is a possible API for the final implementation, which will allow users to compose the component as they see fit:
Currently, only these components are implemented and can be used on the main application:
Screenshots