refactor(widget): make only one flat set of params #3318
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
… docs/widget-lib-1
…wswap into fix/widget-config
apps/widget-configurator/src/app/configurator/hooks/useWidgetParamsAndSettings.ts
Outdated
Show resolved
Hide resolved
libs/widget-lib/docs/README.md
Outdated
| | `theme` | `CowSwapTheme` | The theme of the widget (`'dark'` for dark theme or `'light'` for light theme). | | ||
| | `logoUrl` | `boolean` | The width of the widget in pixels. | | ||
| | `hideLogo` | `boolean` | The height of the widget in pixels. | | ||
| | `hideNetworkSelector` | `boolean` | Disables an opportunity to change the network from the widget UI. | |
There was a problem hiding this comment.
Allow users to change network from the Widget. Missing defaults
I assume is mandatory if we run it in standalone and there's more than one network?
There was a problem hiding this comment.
Since an integrator can preselect a network (by passing chainId parameter) they might want to disable the opportunity. For example, some of integrators supports only Gnosis chain, they wouldn't like their users to switch to Mainnet.
And yes, this option works only in standalone mode.
There was a problem hiding this comment.
@fairlighteth looks like we never show the network selector in the widget mode. Could you please help with that?
libs/widget-lib/docs/README.md
Outdated
| | `hideNetworkSelector` | `boolean` | Disables an opportunity to change the network from the widget UI. | | ||
| | `dynamicHeightEnabled` | `boolean` | Dynamically changes the height of the iframe depending on the content. | | ||
| | `enabledTradeTypes` | `Array<TradeType>` | CowSwap provides three trading widgets: swap, limit and twap orders. Using this option you can narrow down the list of available trading widgets. | | ||
| | `palette` | `CowSwapWidgetPalette` | Using the palette you can customize the appearance of the widget. For example, you can change the main color of the background and text. | |
There was a problem hiding this comment.
We have theme, and palette. Should we make the names closer:
theme: 'dark' | 'light' themePalette: 'dark' | CowSwapWidgetPalette
There was a problem hiding this comment.
I'm afraid we need both.
them sets a general subset of colors.
palette only overrides some of them
There was a problem hiding this comment.
yep, but i could see that in the future palette has more and more options.
BEcause both are part of theming, as a dev i find it easier to find if they show close to each other so their names are similar.
So i was suggesting leaving both: theme & themePallete
However, there's another alternative, since apps will likely setup this once.
What about defining it like this?
type ThemeBase = 'light' | 'dark' | 'auto'
export interface CowSwapTheme {
baseTheme: ThemeBase
primaryColor: string
screenBackground: string
widgetBackground: string
textColor: string
}Then, the theme type would be
{
theme: ThemeBase | CowSwapTheme
...
}
// so users can do:
theme = 'light'
theme = 'dark'
theme = {
baseTheme: 'light,
primaryColor: '#fff'
...
}Alternatively:
type ThemeBase = 'light' | 'dark' | 'auto'
export interface CowSwapTheme {
baseTheme?: ThemeBase
primaryColor?: string
screenBackground?: string
widgetBackground?: string
textColor?: string
}
{
theme?: CowSwapTheme // defaults to "{ baseTheme: 'auto' }" which uses the OS
...
}There was a problem hiding this comment.
Got your point.
Don't you mind if I will implement this in the next PR?
Anyway, currently the palette just randomly bound to colors. It needs and input from Michel
|
… fix/widget-config
|
Merging the PR, the rest of comments will be addressed in the next iteration |
Summary
To simplify the widget lib interface I merged all parameters into one set and made all of the optional.
Now it's actually one line: