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.

Sign up for GitHub

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

Jump to bottom

Be more accepting of select options #1604

Merged
merged 6 commits into from
Feb 1, 2023
Merged

Be more accepting of select options #1604

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
642b2fe
Be more accepting of select options
Janpot Jan 27, 2023
daad6be
Merge branch 'master' into select
Janpot Jan 29, 2023
6a157ba
fix test
Janpot Jan 29, 2023
6bd8f68
fixz
Janpot Feb 1, 2023
ffbb0aa
Merge branch 'master' into select
Janpot Feb 1, 2023
258024f
remove unnecessary
Janpot Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions packages/toolpad-components/src/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ function Select({ options, value, onChange, defaultValue, fullWidth, sx, ...rest
[onChange],
);

const id = React.useId();

return (
<TextField
select
Expand All @@ -31,11 +33,12 @@ function Select({ options, value, onChange, defaultValue, fullWidth, sx, ...rest
onChange={handleChange}
{...rest}
>
{options.map((option) => {
const parsedOption: SelectOption = typeof option === 'string' ? { value: option } : option;
{options.map((option, i) => {
const parsedOption: SelectOption =
option && typeof option === 'object' ? option : { value: String(option) };
return (
<MenuItem key={parsedOption.value} value={parsedOption.value}>
{parsedOption.label ?? parsedOption.value}
<MenuItem key={parsedOption.value ?? `${id}::${i}`} value={parsedOption.value}>
{String(parsedOption.label ?? parsedOption.value)}
</MenuItem>
);
})}
Expand Down
69 changes: 57 additions & 12 deletions test/integration/components/componentsDom.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@
"parentProp": "children",
"parentIndex": "a0"
},
"1m13ljr": {
"id": "1m13ljr",
"name": "codeComponent_eb03t9a",
"type": "element",
"props": {},
"layout": {},
"parentId": "rx23l68",
"attributes": {
"component": {
"type": "const",
"value": "codeComponent.eb03t9a"
}
},
"parentProp": "children",
"parentIndex": "a0"
},
"2m53pfl": {
"id": "2m53pfl",
"name": "pageRow5",
Expand Down Expand Up @@ -173,7 +189,7 @@
"attributes": {
"component": {
"type": "const",
"value": "Typography"
"value": "Text"
}
},
"parentProp": "children",
Expand Down Expand Up @@ -240,6 +256,22 @@
"parentProp": "children",
"parentIndex": "a0"
},
"rx23l68": {
"id": "rx23l68",
"name": "pageRow",
"type": "element",
"props": {},
"layout": {},
"parentId": "f703ps3",
"attributes": {
"component": {
"type": "const",
"value": "PageRow"
}
},
"parentProp": "children",
"parentIndex": "a1V"
},
"vkowdut": {
"id": "vkowdut",
"name": "Application",
Expand Down Expand Up @@ -281,8 +313,8 @@
"parentProp": "children",
"parentIndex": "a0"
},
"rx23l68": {
"name": "pageRow",
"5c43usr": {
"name": "pageRow1",
"props": {},
"attributes": {
"component": {
Expand All @@ -291,28 +323,41 @@
}
},
"layout": {},
"id": "rx23l68",
"id": "5c43usr",
"type": "element",
"parentId": "f703ps3",
"parentProp": "children",
"parentIndex": "a1V"
"parentIndex": "a6"
},
"1m13ljr": {
"name": "codeComponent_eb03t9a",
"props": {},
"6n23u2h": {
"name": "selectWithOptions",
"props": {
"options": {
"type": "jsExpression",
"value": "[\"one\", 2, { value: 3, label: \"three\" }, { value: 4 }, { noValue: \"foo\" }]\n"
},
"label": {
"type": "const",
"value": "select with options"
},
"fullWidth": {
"type": "const",
"value": true
}
},
"attributes": {
"component": {
"type": "const",
"value": "codeComponent.eb03t9a"
"value": "Select"
}
},
"layout": {},
"id": "1m13ljr",
"id": "6n23u2h",
"type": "element",
"parentId": "rx23l68",
"parentId": "5c43usr",
"parentProp": "children",
"parentIndex": "a0"
}
},
"version": 2
"version": 5
}
43 changes: 32 additions & 11 deletions test/integration/components/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,34 @@
import * as path from 'path';
import { ToolpadEditor } from '../../models/ToolpadEditor';
import { ToolpadRuntime } from '../../models/ToolpadRuntime';
import { FrameLocator, Page, test } from '../../playwright/test';
import { FrameLocator, Page, test, expect } from '../../playwright/test';
import clickCenter from '../../utils/clickCenter';
import { readJsonFile } from '../../utils/fs';
import generateId from '../../utils/generateId';

async function waitForComponents(page: Page | FrameLocator) {
await page.locator('text="foo button"').waitFor({ state: 'visible' });
await page.locator('img[alt="foo image"]').waitFor({ state: 'attached' });
await page.locator('text="foo datagrid column"').waitFor({ state: 'visible' });
await page.locator('text="custom component 1"').waitFor({ state: 'visible' });
await page.locator('label:has-text("foo textfield")').waitFor({ state: 'visible' });
await page.locator('text="foo typography"').waitFor({ state: 'visible' });
await page.locator('label:has-text("foo select")').waitFor({ state: 'visible' });
async function waitForComponents(page: Page, frame: Page | FrameLocator = page, isEditor = false) {
await frame.locator('text="foo button"').waitFor({ state: 'visible' });
Copy link
Member

Choose a reason for hiding this comment

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

I'm also thinking if it would be useful to have comments clarifying which component is being checked in each of these lines? The selector might not always be explicit enough.

Copy link
Member Author

@Janpot Janpot Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll assign those locators to variables with more descriptive names. That makes it self-documenting.

await frame.locator('img[alt="foo image"]').waitFor({ state: 'attached' });
await frame.locator('text="foo datagrid column"').waitFor({ state: 'visible' });
await frame.locator('text="custom component 1"').waitFor({ state: 'visible' });
await frame.locator('label:has-text("foo textfield")').waitFor({ state: 'visible' });
await frame.locator('text="foo typography"').waitFor({ state: 'visible' });
await frame.locator('label:has-text("foo select")').waitFor({ state: 'visible' });

const optionsSelect = frame.getByRole('button', { name: /select with options/ });
Copy link
Member

Choose a reason for hiding this comment

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

Should this more specific testing for the Select be in its own test?
Just because so far what this test was doing was just checking that every component renders in a very simple way, but now it's becoming a bit more complex.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, I'm sort of avoiding splitting tests too much as to keep the footprint down. The whole test with setup and page load takes about 2/3 seconds. While just interacting with this select takes an order of magnitude less.
But perhaps it is sensible to add one test per component

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's right, with a page load in every test things can get a lot slower...
One test per component sounds like a good future-proof approach - we could also maybe slowly get rid of this general test as we create one test for each component.
I think the way you changed it is good!

await optionsSelect.scrollIntoViewIfNeeded();
if (isEditor) {
await clickCenter(page, optionsSelect);
}
await clickCenter(page, optionsSelect);
await expect(frame.getByRole('option', { name: 'one' })).toBeVisible();
await expect(frame.getByRole('option', { name: '2' })).toBeVisible();
await expect(frame.getByRole('option', { name: 'three' })).toBeVisible();
await expect(frame.getByRole('option', { name: '4' })).toBeVisible();
await expect(frame.getByRole('option', { name: 'undefined' })).toBeVisible();
}

test('components', async ({ page, browserName, api }) => {
test('components runtime', async ({ page, api }) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be helpful for the test names to be more descriptive of what's being tested, like for example must render components in the runtime. Just an example!

const dom = await readJsonFile(path.resolve(__dirname, './componentsDom.json'));

const app = await api.mutation.createApp(`App ${generateId()}`, {
Expand All @@ -26,9 +39,17 @@ test('components', async ({ page, browserName, api }) => {
await runtimeModel.gotoPage(app.id, 'components');

await waitForComponents(page);
});

test('components editor', async ({ page, browserName, api }) => {
const dom = await readJsonFile(path.resolve(__dirname, './componentsDom.json'));

const app = await api.mutation.createApp(`App ${generateId()}`, {
from: { kind: 'dom', dom },
});

const editorModel = new ToolpadEditor(page, browserName);
editorModel.goto(app.id);

await waitForComponents(editorModel.appCanvas);
await waitForComponents(page, editorModel.appCanvas, true);
});