Fix/chip right icon margin #2772
Conversation
|
Created this pull request so we can review if the margins are good enough. |
src/components/chip/index.tsx
Outdated
| if (label || leftElement || rightElement) { | ||
| return { | ||
| marginLeft: Spacings.s1, | ||
| marginRight: Spacings.s2 | ||
| }; |
There was a problem hiding this comment.
overall It looks much better.
But I think it mostly solve the right icon
notice that the use can either pass rightIconSource or iconSource (rendered on the left)
I think those margins works good for right icons and maybe should be opposite for left icon
… there are icons/elements of both sides of the chip
src/components/chip/index.tsx
Outdated
| if (iconSource || leftElement || rightIconSource || rightElement) { | ||
| const marginFromElement = Spacings.s1; | ||
| if ((iconSource || leftElement) && (rightIconSource || rightElement)) { | ||
| return { | ||
| marginHorizontal: marginFromElement | ||
| }; | ||
| } | ||
| if (iconSource || leftElement) { | ||
| return { | ||
| marginLeft: marginFromElement, | ||
| marginRight: Spacings.s3 | ||
| }; | ||
| } | ||
| if (rightIconSource || rightElement) { | ||
| return { | ||
| marginLeft: Spacings.s3, | ||
| marginRight: marginFromElement | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
A small suggestion for making this a little more readable
you can define two consts
const addLeftMargin = !!(iconSource || leftElement)
const addRightMargin = !!(rightIconSource || rightElement)
Then the conditions will much more readable. Also if there will new elements in either left or right we can simply add them to the const definition and that's it
Last, consider using ternary to make things a little shorter, for example
const marginStyle = {
marginHorizontal: addLeftMargin && addLeftMargin ? Spacings.1 : 0,
marginLeft: ...
marginRight: ...
}
``
There was a problem hiding this comment.
Did you mean
marginHorizontal: addLeftMargin && addRightMargin ? Spacings.s1 : 0?
About the last part. I don't really understand what you mean, If I use marginHorizontal and margin left or right in the same object marginHorizontal will always be overridden by the left and right margins. So I don't really know how to use them together in the same object.
…argin' into fix/Chip-RightIconMargin
expoDemo/package.json
Outdated
There was a problem hiding this comment.
Can you revert this file changes, it's not relevant to the PR
There was a problem hiding this comment.
Weird. Idk where it came from. Done
This reverts commit 2507794.
Description
Added horizontal margins to the right icon in the chip component. The icon was really close to the label when rendering the icon without setting margins manually.
Changelog
Chip - Added default horizontal margins to right icon.
Additional info
WOAUILIB-3772