Skip to content

Commit 4f361e2

Browse files
Copilothotlong
andcommitted
Fix all code review issues - improve type safety, remove unused code, fix accessibility checks
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
1 parent 6988d8d commit 4f361e2

File tree

7 files changed

+17
-71
lines changed

7 files changed

+17
-71
lines changed

packages/components/src/__tests__/README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ Tests for form components:
2424
- Button, Input, Textarea, Select, Checkbox, Switch
2525
- Radio Group, Slider, Label, Email, Password
2626

27-
**Coverage:** 49 tests
27+
**Coverage:** 32 tests
2828

2929
### `layout-data-renderers.test.tsx`
3030
Tests for layout and data display components:
@@ -38,14 +38,14 @@ Tests for feedback and overlay components:
3838
- Feedback: Loading, Spinner, Progress, Skeleton, Empty, Toast
3939
- Overlay: Dialog, Alert Dialog, Sheet, Drawer, Popover, Tooltip, Dropdown Menu, Context Menu, Hover Card, Menubar
4040

41-
**Coverage:** 23 tests
41+
**Coverage:** 35 tests
4242

4343
### `complex-disclosure-renderers.test.tsx`
4444
Tests for complex and disclosure components:
4545
- Disclosure: Accordion, Collapsible, Toggle Group
4646
- Complex: Timeline, Data Table, Chatbot, Carousel, Scroll Area, Resizable, Filter Builder, Calendar View, Table
4747

48-
**Coverage:** 23 tests
48+
**Coverage:** 31 tests
4949

5050
## Running Tests
5151

packages/components/src/__tests__/basic-renderers.test.tsx

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,10 @@
77
*/
88

99
import { describe, it, expect, beforeAll } from 'vitest';
10-
import { screen } from '@testing-library/react';
11-
import { ComponentRegistry } from '@object-ui/core';
1210
import {
1311
renderComponent,
1412
validateComponentRegistration,
1513
getAllDisplayIssues,
16-
checkAccessibility,
1714
checkDOMStructure,
1815
} from './test-utils';
1916

@@ -67,7 +64,6 @@ describe('Basic Renderers - Display Issue Detection', () => {
6764
});
6865

6966
it('should render with designer props correctly', () => {
70-
const Component = ComponentRegistry.get('text');
7167
const { container } = renderComponent(
7268
{ type: 'text', content: 'Designer Test' },
7369
);

packages/components/src/__tests__/complex-disclosure-renderers.test.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { ComponentRegistry } from '@object-ui/core';
1111
import {
1212
renderComponent,
1313
validateComponentRegistration,
14-
getAllDisplayIssues,
1514
checkDOMStructure,
1615
} from './test-utils';
1716

@@ -86,7 +85,7 @@ describe('Disclosure Renderers - Display Issue Detection', () => {
8685
it('should render toggle group with items', () => {
8786
const { container } = renderComponent({
8887
type: 'toggle-group',
89-
type_mode: 'single',
88+
selectionType: 'single',
9089
items: [
9190
{ value: 'bold', label: 'Bold' },
9291
{ value: 'italic', label: 'Italic' },

packages/components/src/__tests__/feedback-overlay-renderers.test.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
*/
88

99
import { describe, it, expect, beforeAll } from 'vitest';
10-
import { ComponentRegistry } from '@object-ui/core';
1110
import {
1211
renderComponent,
1312
validateComponentRegistration,

packages/components/src/__tests__/form-renderers.test.tsx

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,10 @@
88

99
import { describe, it, expect, beforeAll } from 'vitest';
1010
import { screen } from '@testing-library/react';
11-
import userEvent from '@testing-library/user-event';
12-
import { ComponentRegistry } from '@object-ui/core';
1311
import {
1412
renderComponent,
1513
validateComponentRegistration,
1614
getAllDisplayIssues,
17-
checkAccessibility,
1815
} from './test-utils';
1916

2017
// Import renderers to ensure registration

packages/components/src/__tests__/layout-data-renderers.test.tsx

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,9 @@
77
*/
88

99
import { describe, it, expect, beforeAll } from 'vitest';
10-
import { ComponentRegistry } from '@object-ui/core';
1110
import {
1211
renderComponent,
1312
validateComponentRegistration,
14-
getAllDisplayIssues,
1513
checkDOMStructure,
1614
} from './test-utils';
1715

packages/components/src/__tests__/test-utils.tsx

Lines changed: 13 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,12 @@ export function checkAccessibility(element: HTMLElement): {
5050

5151
// Check for form inputs without labels
5252
if (element.tagName === 'INPUT' || element.tagName === 'TEXTAREA' || element.tagName === 'SELECT') {
53-
const hasLabel = hasAriaLabel || element.hasAttribute('id');
53+
const id = element.getAttribute('id');
54+
const doc = element.ownerDocument || document;
55+
const hasAssociatedLabel =
56+
!!element.closest('label') ||
57+
(!!id && !!doc.querySelector(`label[for="${id}"]`));
58+
const hasLabel = hasAriaLabel || hasAssociatedLabel;
5459
if (!hasLabel) {
5560
issues.push('Form element missing label association');
5661
}
@@ -64,30 +69,6 @@ export function checkAccessibility(element: HTMLElement): {
6469
};
6570
}
6671

67-
/**
68-
* Check if element has proper styling classes
69-
*/
70-
export function checkStyling(element: HTMLElement): {
71-
hasClasses: boolean;
72-
hasTailwindClasses: boolean;
73-
hasInlineStyles: boolean;
74-
classes: string[];
75-
} {
76-
const classes = Array.from(element.classList);
77-
const hasClasses = classes.length > 0;
78-
const hasTailwindClasses = classes.some(cls =>
79-
/^(text-|bg-|border-|p-|m-|flex|grid|rounded|shadow|hover:|focus:)/.test(cls)
80-
);
81-
const hasInlineStyles = element.hasAttribute('style') && element.getAttribute('style') !== '';
82-
83-
return {
84-
hasClasses,
85-
hasTailwindClasses,
86-
hasInlineStyles,
87-
classes,
88-
};
89-
}
90-
9172
/**
9273
* Check DOM structure for common issues
9374
*/
@@ -134,34 +115,6 @@ export function checkDOMStructure(container: HTMLElement): {
134115
};
135116
}
136117

137-
/**
138-
* Check if component handles conditional rendering correctly
139-
*/
140-
export function checkConditionalRendering(
141-
baseProps: any,
142-
conditionalProp: string,
143-
conditionalValue: any
144-
): {
145-
baseRender: ReturnType<typeof render>;
146-
conditionalRender: ReturnType<typeof render>;
147-
isDifferent: boolean;
148-
} {
149-
const schema = { type: 'div', ...baseProps };
150-
const conditionalSchema = { ...schema, [conditionalProp]: conditionalValue };
151-
152-
const baseRender = renderComponent(schema);
153-
const conditionalRender = renderComponent(conditionalSchema);
154-
155-
const isDifferent =
156-
baseRender.container.innerHTML !== conditionalRender.container.innerHTML;
157-
158-
return {
159-
baseRender,
160-
conditionalRender,
161-
isDifferent,
162-
};
163-
}
164-
165118
/**
166119
* Validate component registration
167120
*/
@@ -172,7 +125,7 @@ export function validateComponentRegistration(componentType: string): {
172125
hasLabel: boolean;
173126
hasInputs: boolean;
174127
hasDefaultProps: boolean;
175-
config: any;
128+
config: ReturnType<typeof ComponentRegistry.getConfig>;
176129
} {
177130
const isRegistered = ComponentRegistry.has(componentType);
178131
const renderer = ComponentRegistry.get(componentType);
@@ -214,8 +167,12 @@ export function getAllDisplayIssues(container: HTMLElement): string[] {
214167
// This is a simplified check - in React, keys are not in the DOM
215168
// but we can check for duplicate content which might indicate missing keys
216169
const contents = Array.from(items).map(item => item.textContent);
217-
const duplicates = contents.filter((item, index) => contents.indexOf(item) !== index);
218-
if (duplicates.length > 0) {
170+
const contentCounts = new Map<string, number>();
171+
contents.forEach(content => {
172+
contentCounts.set(content || '', (contentCounts.get(content || '') || 0) + 1);
173+
});
174+
const hasDuplicates = Array.from(contentCounts.values()).some(count => count > 1);
175+
if (hasDuplicates) {
219176
issues.push(`Potential duplicate list items detected`);
220177
}
221178
}

0 commit comments

Comments
 (0)