Skip to content
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

fix(chip): update padding for transparent variant (#7164) #8823

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

muraliSingh7
Copy link
Contributor

This PR addresses the issue described in #7164, where the Chip component's padding for the Transparent variant was not correctly aligned. The following changes have been made:

The padding-left for the Transparent variant is now set to 2px to improve alignment.
Other chip variants retain the default horizontal padding values.

These changes ensure that the Transparent chip variant is rendered consistently with the rest of the design, as described in #7164.

See screenshots below for before and after comparison:
Before:
image
image

After:
Screenshot 2024-12-01 at 4 02 08 AM
image

Please review @Bonapara and let me know if you have any feedback or concerns.

Changed the padding-left for the chip component when the variant is Transparent.
The padding-left is now set to 2px to ensure proper alignment, while other variants
continue to use the default horizontal padding.
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Updated padding for the Transparent variant of the Chip component to improve alignment in kanban cards and record pages.

  • Modified packages/twenty-ui/src/display/chip/components/Chip.tsx to set left padding to 2px for Transparent variant
  • Asymmetric padding applied only to left side to match design requirements for 'Created by' chip
  • Used !important flag to override existing padding rules, which may need review for maintainability
  • Change affects only Transparent variant while preserving default horizontal padding for other variants

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 120 to 123
padding-left: ${({ variant }) =>
variant === ChipVariant.Transparent
? '2px !important'
: 'var(--chip-horizontal-padding)'};
Copy link
Contributor

Choose a reason for hiding this comment

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

style: using !important should be avoided - consider restructuring CSS specificity instead

@Bonapara
Copy link
Member

Bonapara commented Dec 2, 2024

Thanks for the PR @muraliSingh7!

On Figma, the padding-left of the table cells should be 8px. It looks like it's 6px today. Can you fix that at the same time?

Desired

CleanShot 2024-12-02 at 09 23 16

Current

CleanShot 2024-12-02 at 09 25 11

@muraliSingh7
Copy link
Contributor Author

@Bonapara I think I don't have access to edit the Figma files. However, I have made some styling changes directly in the code, which changes the padding to 6px.

@Bonapara
Copy link
Member

Bonapara commented Dec 2, 2024

I think we misunderstood each other, @muraliSingh7. The padding-left is currently 6px and should be changed to 8px (Figma). Thanks!

@muraliSingh7
Copy link
Contributor Author

@Bonapara I don't have the necessary permissions to edit Figma files. You may reach out to your team members for that. Could you please tag someone from the UI development team to review this UI code fix ?

@Bonapara
Copy link
Member

Bonapara commented Dec 2, 2024

I think we confused each other @muraliSingh7. It should be changed to 8px in the code, not in Figma.

- Increased padding-left for table cells from 6px to 8px for uniform spacing.
- Set padding to 0px for Chip component in transparent mode to prevent it from affecting cell padding and ensure consistent 8px left padding across all cells.
@muraliSingh7
Copy link
Contributor Author

muraliSingh7 commented Dec 2, 2024

@Bonapara now I understand you want 8px padding-left across all cells. Am I right now?
I have modified styling in two files I hope this fix the issue.

Screenshots after modification :
image
image

@Bonapara
Copy link
Member

Bonapara commented Dec 3, 2024

That's it! Thanks @muraliSingh7


padding-left: ${({ variant }) =>
variant === ChipVariant.Transparent
? '0px'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use theme.spacing(0);

Copy link
Contributor Author

@muraliSingh7 muraliSingh7 Dec 3, 2024

Choose a reason for hiding this comment

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

@Bonapara please check I have updated it to theme.spacing(0)

@guillim
Copy link
Contributor

guillim commented Dec 11, 2024

LGTM. Thanks for your code @muraliSingh7 !!! 🙏

@guillim guillim enabled auto-merge (squash) December 11, 2024 13:34
@guillim guillim self-requested a review December 11, 2024 13:52
@guillim guillim merged commit 6067852 into twentyhq:main Dec 11, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants