-
Notifications
You must be signed in to change notification settings - Fork 889
fix: border + borderRadius rendering issue #2306
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
base: canary
Are you sure you want to change the base?
fix: border + borderRadius rendering issue #2306
Conversation
|
|
@Sagarbisht99 is attempting to deploy a commit to the resend Team on Vercel. A member of the Team first needs to authorize it. |
commit: |
3ff8597 to
3c4ea0a
Compare
7da803f to
e9af285
Compare
49804e6 to
8525a20
Compare
| * Creates a wrapper table that simulates border with background color and padding | ||
| * This approach provides full border-radius support across all email clients | ||
| */ | ||
| export const BorderWrapper: React.FC<BorderWrapperProps> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is interesting, but why only have it for the Section? I think this should be a separate componente, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
17 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/section/src/utils/border-wrapper.spec.tsx">
<violation number="1" location="packages/section/src/utils/border-wrapper.spec.tsx:14">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'returns false when only border is provided'.</violation>
<violation number="2" location="packages/section/src/utils/border-wrapper.spec.tsx:24">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'returns true when both border and borderRadius are provided'.</violation>
<violation number="3" location="packages/section/src/utils/border-wrapper.spec.tsx:33">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'detects individual border properties'.</violation>
<violation number="4" location="packages/section/src/utils/border-wrapper.spec.tsx:56">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'returns null when no style is provided'.</violation>
<violation number="5" location="packages/section/src/utils/border-wrapper.spec.tsx:66">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'extracts border properties when present'.</violation>
<violation number="6" location="packages/section/src/utils/border-wrapper.spec.tsx:93">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'renders children directly when no border properties are present'.</violation>
<violation number="7" location="packages/section/src/utils/border-wrapper.spec.tsx:104">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'wraps with table when border properties are present'.</violation>
<violation number="8" location="packages/section/src/utils/border-wrapper.spec.tsx:119">
P2: Weak assertion could produce false positives. `toContain('padding:1')` will match `padding:10px`, `padding:100px`, etc. Consider using a more precise assertion like `toContain('padding:1px')` or a regex pattern to ensure the exact padding value is verified.</violation>
<violation number="9" location="packages/section/src/utils/border-wrapper.spec.tsx:144">
P2: Rule violated: **No `should` in tests**
Use declarative test description instead of 'should'. Change to 'preserves non-border styles on inner element'.</violation>
</file>
<file name="packages/section/src/section.spec.tsx">
<violation number="1" location="packages/section/src/section.spec.tsx:5">
P2: Rule violated: **No `should` in tests**
Test description contains 'should'. Use declarative language: 'renders correctly'</violation>
<violation number="2" location="packages/section/src/section.spec.tsx:14">
P2: Rule violated: **No `should` in tests**
Test description contains 'should'. Use declarative language: 'renders with style'</violation>
<violation number="3" location="packages/section/src/section.spec.tsx:23">
P2: Rule violated: **No `should` in tests**
Test description contains 'should'. Use declarative language: 'renders with props'</violation>
<violation number="4" location="packages/section/src/section.spec.tsx:32">
P2: Rule violated: **No `should` in tests**
Test description contains 'should'. Use declarative language: 'uses BorderWrapper when both border and borderRadius are present'</violation>
<violation number="5" location="packages/section/src/section.spec.tsx:53">
P2: Rule violated: **No `should` in tests**
Test description contains 'should'. Use declarative language: 'does not use BorderWrapper when only border is present'</violation>
<violation number="6" location="packages/section/src/section.spec.tsx:89">
P2: Rule violated: **No `should` in tests**
Test description contains 'should'. Use declarative language: 'handles individual border properties with borderRadius'</violation>
</file>
<file name="packages/section/src/utils/border-wrapper.tsx">
<violation number="1" location="packages/section/src/utils/border-wrapper.tsx:91">
P1: The component wraps content whenever ANY border property exists, but should only wrap when BOTH border AND borderRadius are present. Use the existing `hasBorderAndBorderRadius` function to check the condition properly.</violation>
<violation number="2" location="packages/section/src/utils/border-wrapper.tsx:132">
P2: If `borderColor` cannot be extracted from the shorthand (e.g., non-solid borders like `dashed` or `dotted`), it will be `undefined`. The wrapper would render without a background color, breaking the border simulation entirely. Consider adding a fallback color or skipping the wrapper when color cannot be determined.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const borderProps = extractBorderProperties(style); | ||
|
|
||
| if (!borderProps) { | ||
| // No border properties, render children directly | ||
| return <>{children}</>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: The component wraps content whenever ANY border property exists, but should only wrap when BOTH border AND borderRadius are present. Use the existing hasBorderAndBorderRadius function to check the condition properly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/utils/border-wrapper.tsx, line 91:
<comment>The component wraps content whenever ANY border property exists, but should only wrap when BOTH border AND borderRadius are present. Use the existing `hasBorderAndBorderRadius` function to check the condition properly.</comment>
<file context>
@@ -0,0 +1,149 @@
+ style,
+ ...props
+}) => {
+ const borderProps = extractBorderProperties(style);
+
+ if (!borderProps) {
</file context>
| const borderProps = extractBorderProperties(style); | |
| if (!borderProps) { | |
| // No border properties, render children directly | |
| return <>{children}</>; | |
| } | |
| if (!hasBorderAndBorderRadius(style)) { | |
| // No border + borderRadius combination, render children directly | |
| return <>{children}</>; | |
| } | |
| const borderProps = extractBorderProperties(style); |
| expect(result).toContain('border-radius:4px'); | ||
| }); | ||
|
|
||
| it('should preserve non-border styles on inner element', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Rule violated: No should in tests
Use declarative test description instead of 'should'. Change to 'preserves non-border styles on inner element'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/utils/border-wrapper.spec.tsx, line 144:
<comment>Use declarative test description instead of 'should'. Change to 'preserves non-border styles on inner element'.</comment>
<file context>
@@ -0,0 +1,165 @@
+ expect(result).toContain('border-radius:4px');
+ });
+
+ it('should preserve non-border styles on inner element', async () => {
+ const result = await render(
+ <BorderWrapper
</file context>
| it('should preserve non-border styles on inner element', async () => { | |
| it('preserves non-border styles on inner element', async () => { |
| expect(result).not.toContain('<table'); | ||
| }); | ||
|
|
||
| it('should wrap with table when border properties are present', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Rule violated: No should in tests
Use declarative test description instead of 'should'. Change to 'wraps with table when border properties are present'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/utils/border-wrapper.spec.tsx, line 104:
<comment>Use declarative test description instead of 'should'. Change to 'wraps with table when border properties are present'.</comment>
<file context>
@@ -0,0 +1,165 @@
+ expect(result).not.toContain('<table');
+ });
+
+ it('should wrap with table when border properties are present', async () => {
+ const result = await render(
+ <BorderWrapper
</file context>
| it('should wrap with table when border properties are present', async () => { | |
| it('wraps with table when border properties are present', async () => { |
| }); | ||
|
|
||
| describe('BorderWrapper component', () => { | ||
| it('should render children directly when no border properties are present', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Rule violated: No should in tests
Use declarative test description instead of 'should'. Change to 'renders children directly when no border properties are present'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/utils/border-wrapper.spec.tsx, line 93:
<comment>Use declarative test description instead of 'should'. Change to 'renders children directly when no border properties are present'.</comment>
<file context>
@@ -0,0 +1,165 @@
+ });
+
+ describe('BorderWrapper component', () => {
+ it('should render children directly when no border properties are present', async () => {
+ const result = await render(
+ <BorderWrapper style={{ color: 'red' }}>
</file context>
| it('should render children directly when no border properties are present', async () => { | |
| it('renders children directly when no border properties are present', async () => { |
| ); | ||
| }); | ||
|
|
||
| it('should extract border properties when present', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Rule violated: No should in tests
Use declarative test description instead of 'should'. Change to 'extracts border properties when present'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/utils/border-wrapper.spec.tsx, line 66:
<comment>Use declarative test description instead of 'should'. Change to 'extracts border properties when present'.</comment>
<file context>
@@ -0,0 +1,165 @@
+ );
+ });
+
+ it('should extract border properties when present', () => {
+ const style = {
+ border: '2px solid red',
</file context>
| it('should extract border properties when present', () => { | |
| it('extracts border properties when present', () => { |
| const html = await render( | ||
| <Section data-testid="section-test" style={style}> | ||
| Test | ||
| it('should render with props', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Rule violated: No should in tests
Test description contains 'should'. Use declarative language: 'renders with props'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/section.spec.tsx, line 23:
<comment>Test description contains 'should'. Use declarative language: 'renders with props'</comment>
<file context>
@@ -1,56 +1,109 @@
- const html = await render(
- <Section data-testid="section-test" style={style}>
- Test
+ it('should render with props', async () => {
+ const actualOutput = await render(
+ <Section align="center" width="100%">
</file context>
| it('should render with props', async () => { | |
| it('renders with props', async () => { |
| const testMessage = 'Test message'; | ||
| const html = await render(<Section>{testMessage}</Section>); | ||
| expect(html).toContain(testMessage); | ||
| it('should render with style', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Rule violated: No should in tests
Test description contains 'should'. Use declarative language: 'renders with style'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/section.spec.tsx, line 14:
<comment>Test description contains 'should'. Use declarative language: 'renders with style'</comment>
<file context>
@@ -1,56 +1,109 @@
- const testMessage = 'Test message';
- const html = await render(<Section>{testMessage}</Section>);
- expect(html).toContain(testMessage);
+ it('should render with style', async () => {
+ const actualOutput = await render(
+ <Section style={{ backgroundColor: 'red' }}>
</file context>
| it('should render with style', async () => { | |
| it('renders with style', async () => { |
| it('renders correctly', async () => { | ||
| const actualOutput = await render(<Section>Lorem ipsum</Section>); | ||
| describe('Section component', () => { | ||
| it('should render correctly', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Rule violated: No should in tests
Test description contains 'should'. Use declarative language: 'renders correctly'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/section.spec.tsx, line 5:
<comment>Test description contains 'should'. Use declarative language: 'renders correctly'</comment>
<file context>
@@ -1,56 +1,109 @@
- it('renders correctly', async () => {
- const actualOutput = await render(<Section>Lorem ipsum</Section>);
+describe('Section component', () => {
+ it('should render correctly', async () => {
+ const actualOutput = await render(
+ <Section>
</file context>
| it('should render correctly', async () => { | |
| it('renders correctly', async () => { |
| cellSpacing="0" | ||
| role="presentation" | ||
| style={{ | ||
| backgroundColor: borderColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: If borderColor cannot be extracted from the shorthand (e.g., non-solid borders like dashed or dotted), it will be undefined. The wrapper would render without a background color, breaking the border simulation entirely. Consider adding a fallback color or skipping the wrapper when color cannot be determined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/utils/border-wrapper.tsx, line 132:
<comment>If `borderColor` cannot be extracted from the shorthand (e.g., non-solid borders like `dashed` or `dotted`), it will be `undefined`. The wrapper would render without a background color, breaking the border simulation entirely. Consider adding a fallback color or skipping the wrapper when color cannot be determined.</comment>
<file context>
@@ -0,0 +1,149 @@
+ cellSpacing="0"
+ role="presentation"
+ style={{
+ backgroundColor: borderColor,
+ padding: borderWidth,
+ borderRadius: borderProps.borderRadius,
</file context>
|
|
||
| expect(result).toContain('<table'); | ||
| expect(result).toContain('background-color:black'); | ||
| expect(result).toContain('padding:1'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Weak assertion could produce false positives. toContain('padding:1') will match padding:10px, padding:100px, etc. Consider using a more precise assertion like toContain('padding:1px') or a regex pattern to ensure the exact padding value is verified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/section/src/utils/border-wrapper.spec.tsx, line 119:
<comment>Weak assertion could produce false positives. `toContain('padding:1')` will match `padding:10px`, `padding:100px`, etc. Consider using a more precise assertion like `toContain('padding:1px')` or a regex pattern to ensure the exact padding value is verified.</comment>
<file context>
@@ -0,0 +1,165 @@
+
+ expect(result).toContain('<table');
+ expect(result).toContain('background-color:black');
+ expect(result).toContain('padding:1');
+ expect(result).toContain('border-radius:8px');
+ expect(result).toContain('<div>Test content</div>');
</file context>
| expect(result).toContain('padding:1'); | |
| expect(result).toContain('padding:1px'); |
🛠️ What does this PR do?
This PR resolves a known issue where applying both
borderandborderRadiusdirectly on a<table>leads to inconsistent rendering across email clients, especially in Outlook.📌 Solution: Instead of applying a CSS border directly, this change wraps the inner table inside an outer table with:
backgroundColorset to theborderColorpaddingset to theborderWidthThis technique visually simulates a border while allowing the inner table’s
borderRadiusto render correctly.💻 Before vs After