Skip to content

Fix/chip right icon margin #2772

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Oct 26, 2023
Merged

Fix/chip right icon margin #2772

Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
456c4eb
Added default horizonatl margins to the right icon in chips
nitzanyiz Oct 16, 2023
831d082
Changed margins in chip when right icon is rendered with anything
nitzanyiz Oct 17, 2023
f789ae3
Merge remote-tracking branch 'origin/master' into fix/Chip-RightIconM…
nitzanyiz Oct 17, 2023
3035ebb
Reverted formatting
nitzanyiz Oct 17, 2023
11e685f
Changed margins for the icons in the chip. added margin handling when…
nitzanyiz Oct 18, 2023
c2364a6
Icon margins only handles in the label margins
nitzanyiz Oct 18, 2023
4ff6391
Lowered margin from element
nitzanyiz Oct 18, 2023
2507794
Fix dependencies of expo demo (#2769)
ethanshar Oct 18, 2023
c827784
Merge remote-tracking branch 'refs/remotes/origin/fix/Chip-RightIconM…
nitzanyiz Oct 25, 2023
dec652a
Formattings
nitzanyiz Oct 25, 2023
51e5841
Changes to margin condtions handling
nitzanyiz Oct 25, 2023
b03a7c5
Revert expoDemo/package.json changes
nitzanyiz Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions src/components/chip/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,22 +255,25 @@ const Chip = ({
marginRight: Spacings.s1
};
}
if (rightElement && leftElement) {
return {
marginHorizontal: 2
};
}
if (iconSource || leftElement) {
return {
marginLeft: 2,
marginRight: Spacings.s3
};
}
if (rightIconSource || rightElement) {
return {
marginLeft: Spacings.s3,
marginRight: 2
};
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
};
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ... 
}
``

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (onDismiss) {
return {
Expand Down