Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor(widget): make only one flat set of params #3318
refactor(widget): make only one flat set of params #3318
Changes from 3 commits
4cf6a2d
0ab13a4
35ecfab
16f09fa
c455013
5e5ca6b
a594b34
db3592b
f0c5cdb
243fd0a
b8bff7e
5258b73
00a2f49
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit: I would write
environment
, however i think is more versatile to pass thecowSwapBaseUrl
defaulting tohttps://swap.cow.fi
. As an interface for clients makes more sense to me. Also, local may be different for different people, maybe I have in another port for example.Also this gives the flexibility to point the widget to IPFS or any other web
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.
Added a link to
COWSWAP_URLS
.About IPFS, I think we can easily add an opportiunity for it just when we have a case for it
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 property actually is mostly for us (developers). Partners always should use prod. I can delete it from the docs to avoid confusion
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fairlighteth looks like we never show the network selector in the widget mode. Could you please help with that?
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.
We have theme, and palette. Should we make the names closer:
theme: 'dark' | 'light'
themePalette: 'dark' |CowSwapWidgetPalette
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.
I'm afraid we need both.
them
sets a general subset of colors.palette
only overrides some of themThere 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.
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?
Then, the theme type would be
Alternatively:
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 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