feat: allow integrators to disable uniswap branding #158
Conversation
JFrankfurt
commented
Aug 22, 2022
JFrankfurt
commented
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/components/BrandedFooter.tsx
Outdated
| @@ -27,6 +29,10 @@ const UniswapA = styled(ExternalLink)` | |||
| ` | |||
|
|
|||
| export default memo(function BrandedFooter() { | |||
| const disableBranding = useAtomValue(disableBrandingAtom) | |||
| if (disableBranding) { | |||
There was a problem hiding this comment.
nit: As a guard I'd prefer this as a one-liner (no brackets).
src/components/Swap/index.tsx
Outdated
| @@ -62,6 +63,7 @@ export default function Swap(props: SwapProps) { | |||
| useSyncSwapEventHandlers(props as SwapEventHandlers) | |||
| useSyncTokenDefaults(props as TokenDefaults) | |||
| useSyncWidgetEventHandlers(props as WidgetEventHandlers) | |||
| useSyncBrandingSetting(props as BrandingSetting) | |||
There was a problem hiding this comment.
This should be sync'ed as part of the Widget component, not as part of the Swap component, as it is widget-scoped state.
There was a problem hiding this comment.
I had the same thought, but the AtomProvider is at the Widget component level, so only its children can share atoms
There was a problem hiding this comment.
This will need to be moved eventually (with useSyncWidgetEventHandlers) but I think it's fine for now.
There was a problem hiding this comment.
On L53, SwapProps must extend BrandingSettings so that the type system will enforce that these props are available.
JFrankfurt
force-pushed
the
disable-branding
branch
from
cb41984
to
15721d1
Compare
src/hooks/useSyncBrandingSetting.ts
Outdated
| import { useUpdateAtom } from 'jotai/utils' | ||
| import { useEffect } from 'react' | ||
|
|
||
| export const disableBrandingAtom = atom<boolean>(false) |
There was a problem hiding this comment.
If you expect more branding settings, you should shape the atom for them:
const initialBrandingSettings: BrandingSettings = {}
const brandingSettingsAtom = atom(initialBrandingSettings)
This will also let you simplify your sync hook, so that any future additions to the BrandingSettings type can be effected by just updating the type, and not touching the rest of the implementation.
export default function useSyncBrandingSettings(settings: BrandingSettings): void {
const updateSettings = useUpdateAtom(brandingSettingsAtom)
useEffect(() => updateSettings(settings), [settings, updateSettings])
}
There was a problem hiding this comment.
I don't expect more. Do you?
src/components/Swap/index.tsx
Outdated
| @@ -62,6 +63,7 @@ export default function Swap(props: SwapProps) { | |||
| useSyncSwapEventHandlers(props as SwapEventHandlers) | |||
| useSyncTokenDefaults(props as TokenDefaults) | |||
| useSyncWidgetEventHandlers(props as WidgetEventHandlers) | |||
| useSyncBrandingSetting(props as BrandingSetting) | |||
There was a problem hiding this comment.
This will need to be moved eventually (with useSyncWidgetEventHandlers) but I think it's fine for now.
src/components/Swap/index.tsx
Outdated
| @@ -62,6 +63,7 @@ export default function Swap(props: SwapProps) { | |||
| useSyncSwapEventHandlers(props as SwapEventHandlers) | |||
| useSyncTokenDefaults(props as TokenDefaults) | |||
| useSyncWidgetEventHandlers(props as WidgetEventHandlers) | |||
| useSyncBrandingSetting(props as BrandingSetting) | |||
There was a problem hiding this comment.
On L53, SwapProps must extend BrandingSettings so that the type system will enforce that these props are available.
src/components/Widget.tsx
Outdated
| @@ -97,6 +98,7 @@ export interface WidgetProps extends TransactionEventHandlers { | |||
| tokenList?: string | TokenInfo[] | |||
| width?: string | number | |||
| dialog?: HTMLDivElement | null | |||
| disableBranding?: boolean | |||
There was a problem hiding this comment.
Remove the explicit disableBranding prop, as it's already included in WidgetProps through extends BrandingSettings.
|
🎉 This PR is included in version 2.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
