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

[Multichain] feat: redesign of multiaccounts and context menu for multiaccounts #4125

Merged
merged 7 commits into from
Sep 17, 2024
Merged

[Multichain] feat: redesign of multiaccounts and context menu for multiaccounts #4125

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cc59938
feat: redesign of multiaccounts and context menu for multiaccounts
schmanu Aug 30, 2024
1c59e54
fix: introduce upsertMUltichainAddressBookEntry reducer
schmanu Sep 11, 2024
8437fbc
refactor: extract component
schmanu Sep 11, 2024
9020775
Merge remote-tracking branch 'origin/epic/multichain-safes' into feat…
schmanu Sep 12, 2024
d15bdb6
fix: show error if no chains available
schmanu Sep 12, 2024
5e3bb59
fix: review issues
schmanu Sep 17, 2024
de3c784
Merge remote-tracking branch 'origin/epic/multichain-safes' into feat…
schmanu Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/components/address-book/EntryDialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ function EntryDialog({
address: '',
},
disableAddressInput = false,
chainId,
chainIds,
currentChainId,
}: {
handleClose: () => void
defaultValues?: AddressEntry
disableAddressInput?: boolean
chainId?: string
chainIds?: string[]
currentChainId: string
}): ReactElement {
const dispatch = useAppDispatch()
Expand All @@ -41,7 +41,11 @@ function EntryDialog({
const { handleSubmit, formState } = methods

const submitCallback = handleSubmit((data: AddressEntry) => {
dispatch(upsertAddressBookEntry({ ...data, chainId: chainId || currentChainId }))
if (chainIds) {
chainIds?.forEach((chainId) => dispatch(upsertAddressBookEntry({ ...data, chainId })))
schmanu marked this conversation as resolved.
Show resolved Hide resolved
} else {
dispatch(upsertAddressBookEntry({ ...data, chainId: currentChainId }))
}
handleClose()
})

Expand Down
107 changes: 107 additions & 0 deletions src/components/sidebar/SafeListContextMenu/MultiAccountContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import type { MouseEvent } from 'react'
import { useState, type ReactElement } from 'react'
import ListItemIcon from '@mui/material/ListItemIcon'
import IconButton from '@mui/material/IconButton'
import MoreVertIcon from '@mui/icons-material/MoreVert'
import MenuItem from '@mui/material/MenuItem'
import ListItemText from '@mui/material/ListItemText'

import EntryDialog from '@/components/address-book/EntryDialog'
import EditIcon from '@/public/images/common/edit.svg'
import PlusIcon from '@/public/images/common/plus.svg'
import ContextMenu from '@/components/common/ContextMenu'
import { trackEvent, OVERVIEW_EVENTS, OVERVIEW_LABELS } from '@/services/analytics'
import { SvgIcon } from '@mui/material'
import { AppRoutes } from '@/config/routes'
import router from 'next/router'
import { CreateSafeOnNewChain } from '@/features/multichain/components/CreateSafeOnNewChain'

enum ModalType {
RENAME = 'rename',
ADD_CHAIN = 'add_chain',
}

const defaultOpen = { [ModalType.RENAME]: false, [ModalType.ADD_CHAIN]: false }

const MultiAccountContextMenu = ({
Copy link
Contributor

@clovisdasilvaneto clovisdasilvaneto Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

about the architecture of this component, since it's a button that shows a dropdown with some options which triggers a modal, would be amazing if we can separate the dropdown in generic renderProps component which provides the openMenu function to the rendered child and takes an array of options? something like:

<DropdownMenu items={items}>
{(openMenu) =>  (
    <IconButton data-testid="safe-options-btn" edge="end" size="small" onClick={handleOpenContextMenu}>
      <MoreVertIcon sx={({ palette }) => ({ color: palette.border.main })} />
    </IconButton>
)}
</DropdownMenu>

in this case, items would be a common type like this:

type item = {
 onClick: (e: MouseEvent) => void,
 label: string
 icon?: React.ReactNode
}[]

Copy link
Member Author

Choose a reason for hiding this comment

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

The architecture also comes from the MUI component library. I do not think we can easily rewrite it to this.
So I'll stick to how the examples use the component for the time being.

name,
address,
chainIds,
}: {
name: string
address: string
chainIds: string[]
}): ReactElement => {
const [anchorEl, setAnchorEl] = useState<HTMLElement | undefined>()
clovisdasilvaneto marked this conversation as resolved.
Show resolved Hide resolved
const [open, setOpen] = useState<typeof defaultOpen>(defaultOpen)

const trackingLabel =
router.pathname === AppRoutes.welcome.accounts ? OVERVIEW_LABELS.login_page : OVERVIEW_LABELS.sidebar
Copy link
Member

Choose a reason for hiding this comment

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

This is only needed in handleOpenModal so I suggest moving it there.


const handleOpenContextMenu = (e: MouseEvent<HTMLButtonElement, globalThis.MouseEvent>) => {
e.stopPropagation()
setAnchorEl(e.currentTarget)
}

const handleCloseContextMenu = (event: MouseEvent) => {
event.stopPropagation()
setAnchorEl(undefined)
}

const handleOpenModal =
(type: keyof typeof open, event: typeof OVERVIEW_EVENTS.SIDEBAR_RENAME | typeof OVERVIEW_EVENTS.SIDEBAR_RENAME) =>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(type: keyof typeof open, event: typeof OVERVIEW_EVENTS.SIDEBAR_RENAME | typeof OVERVIEW_EVENTS.SIDEBAR_RENAME) =>
(type: ModalType, event: typeof OVERVIEW_EVENTS.ADD_NEW_NETWORK | typeof OVERVIEW_EVENTS.SIDEBAR_RENAME) =>

Can we use the enum directly for the type?

Copy link
Member

Choose a reason for hiding this comment

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

Also we have OVERVIEW_EVENTS.SIDEBAR_RENAME twice in here

(e: MouseEvent) => {
handleCloseContextMenu(e)
setOpen((prev) => ({ ...prev, [type]: true }))

trackEvent({ ...event, label: trackingLabel })
}

const handleCloseModal = () => {
setOpen(defaultOpen)
}

return (
<>
<IconButton data-testid="safe-options-btn" edge="end" size="small" onClick={handleOpenContextMenu}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<IconButton data-testid="safe-options-btn" edge="end" size="small" onClick={handleOpenContextMenu}>
<IconButton data-testid="safe-options-btn" edge="end" size="small" onClick={handleOpenContextMenu} ref={anchorEl}>

<MoreVertIcon sx={({ palette }) => ({ color: palette.border.main })} />
</IconButton>
<ContextMenu anchorEl={anchorEl} open={!!anchorEl} onClose={handleCloseContextMenu}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ContextMenu anchorEl={anchorEl} open={!!anchorEl} onClose={handleCloseContextMenu}>
<ContextMenu anchorEl={anchorEl.current}>

<MenuItem onClick={handleOpenModal(ModalType.RENAME, OVERVIEW_EVENTS.SIDEBAR_RENAME)}>
<ListItemIcon>
<SvgIcon component={EditIcon} inheritViewBox fontSize="small" color="success" />
</ListItemIcon>
<ListItemText data-testid="rename-btn">Rename</ListItemText>
</MenuItem>

<MenuItem onClick={handleOpenModal(ModalType.ADD_CHAIN, OVERVIEW_EVENTS.ADD_NEW_NETWORK)}>
<ListItemIcon>
<SvgIcon component={PlusIcon} inheritViewBox fontSize="small" color="primary" />
</ListItemIcon>
<ListItemText data-testid="add-chain-btn">Add another network</ListItemText>
</MenuItem>
</ContextMenu>

{open[ModalType.RENAME] && (
<EntryDialog
handleClose={handleCloseModal}
defaultValues={{ name, address }}
chainIds={chainIds}
disableAddressInput
/>
)}

{open[ModalType.ADD_CHAIN] && (
<CreateSafeOnNewChain
onClose={handleCloseModal}
currentName={name}
deployedChainIds={chainIds}
open
safeAddress={address}
/>
)}
</>
)
}

export default MultiAccountContextMenu
2 changes: 1 addition & 1 deletion src/components/sidebar/SafeListContextMenu/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ const SafeListContextMenu = ({
<EntryDialog
handleClose={handleCloseModal}
defaultValues={{ name, address }}
chainId={chainId}
chainIds={[chainId]}
disableAddressInput
/>
)}
Expand Down
41 changes: 29 additions & 12 deletions src/components/welcome/MyAccounts/MultiAccountItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
AccordionDetails,
AccordionSummary,
Divider,
Tooltip,
} from '@mui/material'
import SafeIcon from '@/components/common/SafeIcon'
import { OVERVIEW_EVENTS, OVERVIEW_LABELS, trackEvent } from '@/services/analytics'
Expand All @@ -25,11 +26,12 @@ import FiatValue from '@/components/common/FiatValue'
import { type MultiChainSafeItem } from './useAllSafesGrouped'
import MultiChainIcon from '@/public/images/sidebar/multichain-account.svg'
import { shortenAddress } from '@/utils/formatters'
import ExpandMoreIcon from '@mui/icons-material/ExpandMore'
import { type SafeItem } from './useAllSafes'
import SubAccountItem from './SubAccountItem'
import { getSharedSetup } from './utils/multiChainSafe'
import { AddNetworkButton } from './AddNetworkButton'
import ChainIndicator from '@/components/common/ChainIndicator'
import MultiAccountContextMenu from '@/components/sidebar/SafeListContextMenu/MultiAccountContextMenu'

type MultiAccountItemProps = {
multiSafeAccountItem: MultiChainSafeItem
Expand All @@ -46,6 +48,8 @@ const MultiAccountItem = ({ onLinkClick, multiSafeAccountItem, safeOverviews }:
const isWelcomePage = router.pathname === AppRoutes.welcome.accounts
const [expanded, setExpanded] = useState(isCurrentSafe)

const deployedChains = useMemo(() => safes.map((safe) => safe.chainId), [safes])

const isWatchlist = useMemo(
() => multiSafeAccountItem.safes.every((safe) => safe.isWatchlist),
[multiSafeAccountItem.safes],
Expand Down Expand Up @@ -83,24 +87,22 @@ const MultiAccountItem = ({ onLinkClick, multiSafeAccountItem, safeOverviews }:
<ListItemButton
data-testid="safe-list-item"
selected={isCurrentSafe}
className={classnames(css.listItem, { [css.currentListItem]: isCurrentSafe })}
className={classnames(css.multiListItem, css.listItem, { [css.currentListItem]: isCurrentSafe })}
sx={{ p: 0 }}
>
<Accordion expanded={expanded} sx={{ border: 'none' }}>
<AccordionSummary
onClick={toggleExpand}
expandIcon={<ExpandMoreIcon />}
sx={{
pl: 0,
'& .MuiAccordionSummary-content': { m: 0 },
'& .MuiAccordionSummary-content': { m: 0, alignItems: 'center' },
'&.Mui-expanded': { backgroundColor: 'transparent !important' },
}}
>
<Box className={css.safeLink} width="100%">
<Box pr={2.5}>
<SafeIcon address={address} owners={sharedSetup?.owners.length} threshold={sharedSetup?.threshold} />
</Box>

<Typography variant="body2" component="div" className={css.safeAddress}>
{name && (
<Typography variant="subtitle2" component="p" fontWeight="bold" className={css.safeName}>
Expand All @@ -111,20 +113,35 @@ const MultiAccountItem = ({ onLinkClick, multiSafeAccountItem, safeOverviews }:
{shortenAddress(address)}
</Typography>
</Typography>

<Typography variant="body2" fontWeight="bold" textAlign="right" pr={4}>
{totalFiatValue !== undefined ? (
<FiatValue value={totalFiatValue} />
) : (
<Skeleton variant="text" sx={{ ml: 'auto' }} />
)}
</Typography>

<MultiChainIcon />
<Tooltip
title={
<Box>
<Typography fontSize="14px">Multichain account on:</Typography>
{safes.map((safeItem) => (
<Box p="4px 0px" key={safeItem.chainId}>
<ChainIndicator chainId={safeItem.chainId} />
</Box>
))}
</Box>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if we can move it to be a separated component outside the MultiAccountItem? So we just need to call the reference of the component inside the tooltip instead of re-creating the component every-time the parent has a re-render?

}
arrow
>
<Box height="26px">
<MultiChainIcon />
</Box>
</Tooltip>
</Box>
<MultiAccountContextMenu name={name ?? ''} address={address} chainIds={deployedChains} />
</AccordionSummary>
<AccordionDetails sx={{ padding: 0 }}>
<Box sx={{ padding: '0px 0px 0px 36px' }}>
<AccordionDetails sx={{ padding: '0px 12px' }}>
<Box>
{safes.map((safeItem) => (
<SubAccountItem
onLinkClick={onLinkClick}
Expand All @@ -136,8 +153,8 @@ const MultiAccountItem = ({ onLinkClick, multiSafeAccountItem, safeOverviews }:
</Box>
{!isWatchlist && (
<>
<Divider />
<Box display="flex" alignItems="center" justifyContent="center">
<Divider sx={{ ml: '-12px', mr: '-12px' }} />
<Box display="flex" alignItems="center" justifyContent="center" sx={{ ml: '-12px', mr: '-12px' }}>
<AddNetworkButton
currentName={name}
safeAddress={address}
Expand Down
1 change: 0 additions & 1 deletion src/components/welcome/MyAccounts/SubAccountItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ const SubAccountItem = ({ onLinkClick, safeItem, safeOverview }: SubAccountItem)
safeAddress={address}
chainShortName={chain?.shortName || ''}
/>
<div className={css.borderLeft} />
</ListItemButton>
)
}
Expand Down
44 changes: 10 additions & 34 deletions src/components/welcome/MyAccounts/styles.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@
background-color: var(--color-background-light) !important;
}

.currentListItem.multiListItem {
border-left-width: 1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't need this first line, since the next one overwrites this one 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

There is the single account component which uses only currentListItem without the multiListItem css class.
I think its needed there.

Copy link
Contributor

Choose a reason for hiding this comment

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

@schmanu I mean, in the same selector, line 49, you overwrite it with border: 1px solid var(--color-border-light) 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah sorry I misunderstood. I thought you meant the entire block not just the line -.-
You are right. I will remove it :)

border: 1px solid var(--color-border-light);
background-color: none;
}

.listItem :global .MuiAccordion-root,
.listItem :global .MuiAccordion-root:hover > .MuiAccordionSummary-root {
background-color: transparent;
Expand All @@ -54,48 +60,18 @@
}

.listItem.subItem {
border: none;
margin-bottom: 0px;
border-radius: 0px;
}

.subItem:before {
content: '';
display: block;
width: 8px;
height: 1px;
background: var(--color-border-light);
left: 0;
top: 50%;
position: absolute;
}

.subItem.currentListItem {
border: none;
}

.subItem.currentListItem:before {
background: var(--color-secondary-light);
height: 1px;
margin-bottom: 8px;
}

.subItem .borderLeft {
top: 0;
bottom: 0;
position: absolute;
border-left: 1px solid var(--color-border-light);
border-radius: 6px;
border: 1px solid var(--color-border-light);
}
.subItem.currentListItem .borderLeft {
border-left: 1px solid var(--color-secondary-light);
}

.subItem:last-child {
border-left: none;
}

.subItem:last-child .borderLeft {
top: 0%;
bottom: 50%;
border-left: 4px solid var(--color-secondary-light);
}

.listItem > :first-child {
Expand Down
2 changes: 1 addition & 1 deletion src/features/multichain/components/CreateSafeOnNewChain/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export const CreateSafeOnNewChain = ({
const submitDisabled = !!safeCreationDataError

return (
<Dialog open={open} onClose={onClose}>
<Dialog open={open} onClose={onClose} onClick={(e) => e.stopPropagation()}>
<form onSubmit={onFormSubmit} id="recreate-safe">
<DialogTitle fontWeight={700}>Add another network</DialogTitle>
<DialogContent>
Expand Down
Loading