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

added left padding in filter chip #7800

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

SShanks451
Copy link
Contributor

@SShanks451 SShanks451 commented Oct 17, 2024

Fixes: #7779

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

This pull request adds left padding to the SortOrFilterChip component, potentially affecting the spacing and layout of filter chips throughout the application.

  • Modified packages/twenty-front/src/modules/views/components/SortOrFilterChip.tsx to include a left margin of 8px
  • The hardcoded 8px value may not align with the existing theme-based spacing system
  • Consider the impact on overall layout consistency, especially in different contexts where the chip is used
  • Evaluate if this change should be applied universally or if it's addressing a specific use case

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

@@ -42,6 +42,7 @@ const StyledChip = styled.div<{ variant: SortOrFitlerChipVariant }>`
padding: ${({ theme }) => theme.spacing(0.5) + ' ' + theme.spacing(2)};
user-select: none;
white-space: nowrap;
margin-left: 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using theme-based spacing for consistency

Copy link

github-actions bot commented Oct 17, 2024

Welcome!

Hello there, congrats on your first PR! We're excited to have you contributing to this project.
By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement.

Generated by 🚫 dangerJS against 7097f88

@charlesBochet
Copy link
Member

@SShanks451 thanks for the PR, is there any open issue motivating this change? If yes could you link it?

Also, please use the theme instead of a hardcoded pixel value

@SShanks451
Copy link
Contributor Author

@charlesBochet updated the PR...please review

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

LGTM :)

@charlesBochet charlesBochet merged commit c0e6fb6 into twentyhq:main Oct 17, 2024
11 of 13 checks passed
Copy link

oss-gg bot commented Oct 17, 2024

Awarding SShanks451: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/SShanks451

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression on filters Left-margin
2 participants