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

Left Padding removed in Settings Page Tabs #7730

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

shyamsundertard
Copy link
Contributor

@shyamsundertard shyamsundertard commented Oct 16, 2024

Fix: #7100

The TabList component, located in Tablist, wraps the Tabs and defines the padding, and is used in multiple places. The left padding for the Emails and Calendars sections of the Accounts in Settings has been removed ( list appear when there are multiple connected accounts ). However, the padding on the Record detail page remains unchanged.

To address this, prop of css styles is added to Tablist, allowing for the padding of the TabList component to be adjusted as required.

Additional styles can also be applied as per requirements individually for Emails and Calendar section.

Screenshot 2024-10-16 at 5 06 26 AM Screenshot 2024-10-16 at 5 49 18 AM Screenshot 2024-10-16 at 6 22 30 AM

Copy link

github-actions bot commented Oct 16, 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 190c10f

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 introduces changes to improve the layout of the Settings page tabs, focusing on padding adjustments and component flexibility.

  • Added new CustomPaddings component in packages/twenty-front/src/modules/ui/layout/tab/components/CustomPaddings.tsx for dynamic padding control
  • Modified TabList component in packages/twenty-front/src/modules/ui/layout/tab/components/TabList.tsx to use CSS variables for flexible padding
  • Updated SettingsAccountsCalendarChannelsContainer and SettingsAccountsMessageChannelsContainer to use CustomPaddings with zero padding
  • Increased Node.js max old space size from 3000MB to 4096MB in packages/twenty-front/package.json for improved build performance

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

tabListId={SETTINGS_ACCOUNT_CALENDAR_CHANNELS_TAB_LIST_COMPONENT_ID}
tabs={tabs}
/>
<CustomPaddings padding="0">
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 a constant for the padding value '0' for consistency and easier maintenance

interface CustomPaddingsProps {
padding?: string | number;
children: React.ReactNode;
className?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: className prop is defined but not used in the component


return (
<div
className={`custom-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: Consider using the className prop here for better customization

@@ -30,8 +30,9 @@ const StyledContainer = styled.div`
box-sizing: border-box;
display: flex;
gap: ${({ theme }) => theme.spacing(2)};
padding: var(--custom-padding, ${({ theme }) => theme.spacing(2)});
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.spacing() for padding-bottom to maintain consistency with other spacing values

@shyamsundertard
Copy link
Contributor Author

If there are any changes that have to be made in it please let me know.

@shyamsundertard
Copy link
Contributor Author

@Bonapara can you please review this PR

@shyamsundertard
Copy link
Contributor Author

This is my first PR, and I'm learning as I go.

@Bonapara
Copy link
Member

Looks good from a design perspective; I'll leave the code to the tech team.

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.

I've found a simpler fix :)

@charlesBochet
Copy link
Member

Thanks for taking a look @shyamsundertard

@charlesBochet
Copy link
Member

/award 150

Copy link

oss-gg bot commented Oct 21, 2024

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

@charlesBochet charlesBochet merged commit 34ef2d3 into twentyhq:main Oct 21, 2024
12 of 13 checks passed
@shyamsundertard shyamsundertard deleted the fix-settings-page branch November 2, 2024 13:44
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.

Remove 8px Left Padding in Settings Page Tabs
4 participants