feat(Amount): add new props & remove prefix
#1987
Conversation
|
|
✅ PR title follows Conventional Commits specification. |
snitin315
changed the title
prefix\
snitin315
changed the title
prefix\prefix
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7d7db80:
|
snitin315
requested review from
chaitanyadeorukhkar and
kamleshchandnani
as code owners
| * | ||
| * @default false | ||
| */ | ||
| isStrikethrough?: boolean; |
There was a problem hiding this comment.
nit: wouldn't shouldStrikethrough be more suitable?
There was a problem hiding this comment.
All our booleans are either has* or is*. We don't use should* for booleans anywhere. I think isStrikethrough makes sense here
There was a problem hiding this comment.
I'm fine with either isStrikethrough or hasStrikethrough .
| borderBottomStyle="solid" | ||
| position="absolute" | ||
| width="100%" | ||
| top="50%" |
There was a problem hiding this comment.
will top 50% be always correct? we use rems and if we increase or decrease fontsize of browser then I hope the place of line doesn't changes?
There was a problem hiding this comment.
Yes, 50% will always be in the middle of the amount value.
...es/blade/src/components/Collapsible/__tests__/__snapshots__/Collapsible.native.test.tsx.snap
Outdated
Show resolved
Hide resolved
| @@ -84,6 +87,7 @@ export const BaseText = ({ | |||
| textAlign={textAlign} | |||
| numberOfLines={truncateAfterLines} | |||
| wordBreak={wordBreak} | |||
| opacity={opacity} | |||
There was a problem hiding this comment.
I don't think we should expose opacity. This will make base text colors unpredictable and will always rely on the background color. Lets speak with the design team once to confirm this
There was a problem hiding this comment.
As consulted with design, we have now increased the opacity to 64%.
|
it should strikethrough across the whole amount right? So the rupee symbol should also have a strikethrough? But seems like its only the numbers that have it right now 
|
@chaitanyadeorukhkar This is as per the designs. 
|
We have updated it to strike through whole component (including currency-symbol/code) after consulting with the designers. |
| const currencyPositionMapping: Record<string, string> = { | ||
| DZD: 'right', | ||
| BHD: 'right', | ||
| OMR: 'right', | ||
| KWD: 'right', | ||
| }; |
There was a problem hiding this comment.
Will these get filled with more currencies later?
prefixprop in favor of the newcurrencyIndicatorprop.currencyIndicator: Determines the visual representation of the currency (symbol or code).isStrikethrough: If true, the amount value will have a line through it.