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(L2): add Arbitrum UI #1912

Merged
merged 1 commit into from Jun 26, 2021
Merged

feat(L2): add Arbitrum UI #1912

merged 1 commit into from Jun 26, 2021

Conversation

JFrankfurt
Copy link
Contributor

@JFrankfurt JFrankfurt commented Jun 24, 2021 •
edited

image

@JFrankfurt JFrankfurt self-assigned this Jun 24, 2021
@vercel
Copy link

vercel bot commented Jun 24, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/uniswap/uniswap-interface/6kXJeRkM9fmFyehXwrBPdowpdaqG
✅ Preview: https://uniswap-interface-git-fork-jfrankfurt-l2-arbitrum-ui-uniswap.vercel.app

@JFrankfurt JFrankfurt mentioned this pull request Jun 24, 2021
Closed
moodysalem
moodysalem suggested changes Jun 25, 2021
View reviewed changes
src/components/Header/NetworkCard.tsx Outdated
useOnClickOutside(node, open ? toggle : undefined)
const { ethereum } = window
const [implements3085, setImplements3085] = React.useState(false)
React.useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we doing this on mount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to determine if the menu option for switching back to L1 is supported and should be visible. Switching to the current network is a noop and the promise returns null if it's supported, and it throws if the api is unavailable. Do you think we should run the check somewhere else? I could see doing a global check somewhere and retaining the support info for a check here.

Copy link
Contributor

Choose a reason for hiding this comment

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

could just check provider.isMetaMask

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thought occurred to me, but I figured this gave us better flexibility for supporting other clients that might support the EIP now (w/o me knowing of it) or support it in the future w/o code change. Admittedly, it is super gross and I hate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, but sending this request while no op on metamask may produce a strange result in another wallet

src/components/Header/NetworkCard.tsx Outdated Show resolved Hide resolved
src/components/Header/NetworkCard.tsx Outdated Show resolved Hide resolved
src/components/swap/SwapNetworkAlert.tsx Outdated Show resolved Hide resolved
src/hooks/useChainIdBackground.ts Outdated Show resolved Hide resolved
src/hooks/useLogs.ts Outdated Show resolved Hide resolved
src/pages/Swap/index.tsx Outdated
React.useEffect(() => {
const onArbitrum = chainId === SupportedChainId.ARBITRUM_ONE
if (onArbitrum && account && library) {
library.getBalance(account).then((balance) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the balance fetching hook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, didn't know this was a thing

src/pages/Swap/index.tsx Outdated
@@ -68,7 +69,10 @@ const StyledInfo = styled(Info)`
}
`

const ARBITRUM_LOCAL_STORAGE_DISMISSED_KEY = 'HIDE_ARBITRUM_SWAP_ALERT'
Copy link
Contributor

Choose a reason for hiding this comment

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

please use redux for storing this state, for which the user slice is already stored to localstorage

src/react-app-env.d.ts Outdated Show resolved Hide resolved
src/pages/Swap/index.tsx Outdated
@@ -350,6 +353,31 @@ export default function Swap({ history }: RouteComponentProps) {

const priceImpactTooHigh = priceImpactSeverity > 3 && !isExpertMode

// arbitrum network alert
const [arbitrumAlertVisible, setArbitrumAlertVisible] = React.useState(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why you need state for this, just store whether it's dismissed in redux, and you should show it if they're on arbitrum and haven't dismissed it yet

moodysalem
moodysalem reviewed Jun 25, 2021
View reviewed changes
src/hooks/useChainIdBackground.ts Outdated
React.useEffect(() => {
const background = document.querySelector('#background-radial-gradient')
if (background === null) {
const background = document.querySelector('#background-radial-gradient') as HTMLDivElement | null
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, nit, document.getElementById('background-radial-gradient') is faster (not that it matters at all)

Copy link

Choose a reason for hiding this comment

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

getElementById is not faster than querySelector unless maybe you're executing it thousands of times in a loop in IE7 in 2007. Then… maybe 😜

src/hooks/useChainIdBackground.ts Outdated
import React from 'react'
import { useActiveWeb3React } from './web3'

export const useChainIdBackground = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't feel strongly about where it goes

src/components/Header/NetworkCard.tsx Outdated
useOnClickOutside(node, open ? toggle : undefined)
const { ethereum } = window
const [implements3085, setImplements3085] = React.useState(false)
React.useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

sure, but sending this request while no op on metamask may produce a strange result in another wallet

@JFrankfurt JFrankfurt merged commit dfe550c into Uniswap:main Jun 26, 2021
@JFrankfurt JFrankfurt deleted the L2-arbitrum-UI branch June 28, 2021 01:46
daniel-dev1990 pushed a commit to daniel-dev1990/uniswap-interface that referenced this pull request Jul 8, 2021
@JFrankfurt
add arbitrum-specific L2 designs (Uniswap#1912)
85ba814
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@subtleGradient subtleGradient subtleGradient left review comments

@moodysalem moodysalem moodysalem approved these changes

Assignees

@JFrankfurt JFrankfurt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JFrankfurt @subtleGradient @moodysalem