AAE-37713 Amount locale display #11285
Conversation
tomaszhanaj
requested review from
DenysVuika,
eromano and
popovicsandras
as code owners
lib/core/src/lib/form/components/widgets/amount/amount.widget.spec.ts
Outdated
Show resolved
Hide resolved
lib/core/src/lib/form/components/widgets/amount/amount.widget.ts
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| amountWidgetOnBlur() { |
There was a problem hiding this comment.
Can we add return type to all methods
lib/core/src/lib/form/components/widgets/amount/amount.widget.ts
Outdated
Show resolved
Hide resolved
|
|
||
| setInitialValues(): void { | ||
| if (this.enableDisplayBasedOnLocale) { | ||
| this.decimalProperty = this.field.enableFractions ? '1.2-2' : '1.0-0'; |
There was a problem hiding this comment.
can we somehow make this values 1.2-2', 1.0-0 more understandable?
You can check magic number pattern e.g. https://refactoring.guru/replace-magic-number-with-symbolic-constant
A magic number is a numeric value that’s encountered in the source but has no obvious meaning. This “anti-pattern” makes it harder to understand the program and refactor the code.
lib/core/src/lib/form/components/widgets/amount/amount.widget.html
Outdated
Show resolved
Hide resolved
| locale: string; | ||
| showReadonlyPlaceholder: boolean; | ||
| valueAsNumber: number; | ||
| valueAsString: string; |
There was a problem hiding this comment.
Do we need both 🤔 valueAsNumber and valueAsString
There was a problem hiding this comment.
We are only using it here, I do not think we need this additional prop 🤔
this.amountWidgetValue = this.valueAsString;
If the valueAsNumber is only set here this.valueAsNumber = this.field.value;
This is also not needed IMO
There was a problem hiding this comment.
valueAsNumber is needed. it is used in amountWidgetOnFocus() when user enters the field its value is assigned to field. It is set on amountWidgetOnBlur() when user leaves the field it is used to store numeric value and when setting initial values (so it is set in two places). Its intention is to store value from field as number to not convert it from string. But valueAsString can be removed. updated.
BSekula
left a comment
BSekula
left a comment
There was a problem hiding this comment.
Please check comments especially usage of valueAsNumber and valueAsString as these looks redundant
tomaszhanaj
changed the title
tomaszhanaj
changed the title
tomaszhanaj
force-pushed
the
feature/AAE-37713-amount-locale-display
branch
from
fb8c03d to
b934da6
Compare
|
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
What is the new behaviour?
https://hyland.atlassian.net/browse/AAE-37713
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: