-
Notifications
You must be signed in to change notification settings - Fork 102
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
feat: surplus modal #2767
feat: surplus modal #2767
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Looks nice :)
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.
Looks amazing!
One small suggestion regarding the modal size logic
if (activityDerivedState && getActivityState(activityDerivedState) === 'filled') { | ||
return 400 | ||
return 470 |
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.
In that case you can unify the return with line 89
Change the if condition to:
if((activityDerivedState && getActivityState(activityDerivedState) === 'filled') || !hash)
then remove lines 86, 88 and 89, returning 850
in the default case
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.
Or, can we specify the 470 value specifically for this surplus modal? Not sure that's a specific 'activity'.
function getTwitterText(surplusAmount: string, surplusToken: string) { | ||
const surplus = `${surplusAmount} ${surplusToken}` | ||
return encodeURIComponent( | ||
`Hey, I just ${SELL_SURPLUS_WORD} ${surplus} on @CoWSwap! 🐮💸\n\nStart swapping on swap.cow.fi` |
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.
shouldn't this message receive the order.kind
as a parameter (or directly the surplusWord
)? so it works for buy orders too?
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.
Yes you are right. Just committed a fix.
Hey @fairlighteth , great! |
@elena-zh Thank you for reviewing. The first and last point should be fixed. |
@fairlighteth , cases 1, 4 LGTM! |
@elena-zh I will merge and we can see about point 2 and 3 as this PR aims to focus around the styling aspects. |
@fairlighteth I can take care of that if you want |
@fairlighteth , @alfetopito , I have created #2780 task for this |
Summary
onTweetShare
Todo: