-
-
Notifications
You must be signed in to change notification settings - Fork 302
feat(suite): use message system context and features in trading #19089
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
Conversation
|
||
export const TradingDisabled = ({ type }: TradingDisabledProps) => ( | ||
<Banner icon="warning" variant="warning"> | ||
{capitalize(type)} is currently disabled. |
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.
Has to be translatable
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.
but maybe we want to use some message from the message system
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, I know, but I'm not sure what content should be here, so I temporarily added the Banner
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 changed this for content from feature flags and also added a default translation string if the content is missing
a03b0f1
to
644c119
Compare
suite-native/module-trading/src/selectors/__tests__/commonSelectors.test.ts
Show resolved
Hide resolved
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.
LGTM, not tested
644c119
to
668912c
Compare
✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
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.
mobile part looks good 💪. Thanks for aligning the name 🙇
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.
Nice, I like the renaming to exchange
. I have just one thing to think about.
const type: TradingType = 'buy'; | ||
const { isDisabled, content } = useMessageSystemTrading(type); | ||
|
||
const tradingBuyContextValues = useTradingBuyForm({ selectedAccount }); |
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:
const type: TradingType = 'buy'; | |
const { isDisabled, content } = useMessageSystemTrading(type); | |
const tradingBuyContextValues = useTradingBuyForm({ selectedAccount }); | |
const tradingBuyContextValues = useTradingBuyForm({ selectedAccount }); | |
const { isDisabled, content } = useMessageSystemTrading(tradingBuyContextValues.type); |
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.
One more idea to think about.
const tradingBuyContextValues = useTradingBuyForm({ selectedAccount });
will initialize the buy context, and some necessary requests could be fired regardless disabled section. Maybe creating a new component with context initialization could solve the issue?
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 have reworked the components a bit. Every type now has three components - TradingXForm
, TradingXFormWrapper
, TradingXFormContent
. I was thinking about some kind of abstraction, but I realized that would be more overengineering. So, what do you think of the current version?
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 good to me.
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.
Thank you for the updates
packages/suite/src/views/wallet/trading/exchange/TradingExchangeForm.tsx
Outdated
Show resolved
Hide resolved
packages/suite/src/views/wallet/trading/sell/TradingSellForm.tsx
Outdated
Show resolved
Hide resolved
668912c
to
da9efc0
Compare
✅ Previously successful run of [Test] PR Suite Desktop e2e tests workflow has been found. |
config action:
|
Description
Integrate message system context and features in trading on the desktop.
Related Issue
Resolve #18806
Screenshots:
🔍🖥️ Suite web test results: View in Currents
🔍🖥️ Suite desktop test results: View in Currents
🔍🖥️ Suite native android test results: View in Currents