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 Fiat Plugin enterAmount
API
#4186
Conversation
enterAmount
API
src/plugins/gui/amountQuotePlugin.ts
Outdated
@@ -158,7 +158,7 @@ export const amountQuoteFiatPlugin: FiatPluginFactory = async (params: FiatPlugi | |||
getMethods: (methods: FiatPluginGetMethodsResponse) => { | |||
enterAmountMethods = methods | |||
}, | |||
convertValue: async (sourceFieldNum: number, value: string): Promise<string | undefined> => { | |||
onFieldChange: async (sourceFieldNum: number, value: string): Promise<string | undefined> => { |
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 would disagree with this rename. The callback NEEDS to do a value conversion. It's not just a generic field change callback
import { useHandler } from '../../../hooks/useHandler' | ||
import { useState } from '../../../types/reactHooks' | ||
|
||
export class StateManager<State extends object> { |
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.
why the class? Why not just instantiate an object within useRef on line 28?
src/plugins/gui/amountQuotePlugin.ts
Outdated
@@ -157,15 +157,24 @@ export const amountQuoteFiatPlugin: FiatPluginFactory = async (params: FiatPlugi | |||
}, | |||
label1: sprintf(lstrings.fiat_plugin_amount_currencycode, displayFiatCurrencyCode), | |||
label2: sprintf(lstrings.fiat_plugin_amount_currencycode, currencyCode), | |||
onFieldChange: async (sourceFieldNum, value, stateManager): Promise<string | undefined> => { | |||
onFieldChange: async (sourceFieldNum, value, stateManager) => { |
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.
Still not sure I like this convention as it makes it "optional" to update the other field.
const otherFieldKey = sourceFieldNum === 1 ? 'value2' : 'value1' | ||
const spinnerKey = sourceFieldNum === 1 ? 'spinner2' : 'spinner1' | ||
|
||
stateManager.update({ [spinnerKey]: true }) |
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.
why is the spinner now getting modified in this callback?
@@ -129,3 +115,8 @@ export interface FiatPlugin { | |||
} | |||
|
|||
export type FiatPluginFactory = (params: FiatPluginFactoryArgs) => Promise<FiatPlugin> | |||
|
|||
export interface StatefulSceneEvent<EventValue, State extends object> { |
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 won't ask for a change, but not sure what this commit buys us. The syntax for calling the callbacks is more complex. The generics also make the code harder to read.
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.
Approved with request for changes to be in a new PR
19b880f
to
0e803e0
Compare
CHANGELOG
enterAmount
APIDependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: