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

refactor: Update tests to use common test and apply correct default display. #3778

Merged
merged 9 commits into from
Dec 30, 2021
Merged

refactor: Update tests to use common test and apply correct default display. #3778

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d2e49ee
refactor: Update tests to use common test and apply correct default d…
driskull Dec 22, 2021
a59fbd2
fixes
driskull Dec 23, 2021
3e119e3
fixes
driskull Dec 23, 2021
11cf67a
fix some tests
driskull Dec 23, 2021
8cbdf52
fix test
driskull Dec 23, 2021
03bcdac
make display option required
driskull Dec 23, 2021
68e3554
review fixes
driskull Dec 27, 2021
0902327
Merge branch 'master' into dris0000/default-display-tests
driskull Dec 27, 2021
7d9ed8e
Merge branch 'master' into dris0000/default-display-tests
driskull Dec 30, 2021
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
44 changes: 21 additions & 23 deletions src/components/calcite-dropdown/calcite-dropdown.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,29 @@
import { E2EPage, newE2EPage } from "@stencil/core/testing";
import { HYDRATED_ATTR, accessible, defaults } from "../../tests/commonTests";
import { accessible, defaults, renders } from "../../tests/commonTests";
import dedent from "dedent";
import { html } from "../../tests/utils";

describe("calcite-dropdown", () => {
it("renders", () =>
renders(
`
<calcite-dropdown>
<calcite-button slot="dropdown-trigger">Open dropdown</calcite-button>
<calcite-dropdown-group id="group-1">
<calcite-dropdown-item id="item-1">
Dropdown Item Content
</calcite-dropdown-item>
<calcite-dropdown-item id="item-2" active>
Dropdown Item Content
</calcite-dropdown-item>
<calcite-dropdown-item id="item-3">
Dropdown Item Content
</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>`,
{ display: "inline-flex" }
));

it("defaults", async () =>
defaults("calcite-dropdown", [
{
Expand Down Expand Up @@ -58,28 +78,6 @@ describe("calcite-dropdown", () => {
</calcite-dropdown>
`;

it("renders", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-dropdown>
<calcite-button slot="dropdown-trigger">Open dropdown</calcite-button>
<calcite-dropdown-group id="group-1">
<calcite-dropdown-item id="item-1">
Dropdown Item Content
</calcite-dropdown-item>
<calcite-dropdown-item id="item-2" active>
Dropdown Item Content
</calcite-dropdown-item>
<calcite-dropdown-item id="item-3">
Dropdown Item Content
</calcite-dropdown-item>
</calcite-dropdown-group>
</calcite-dropdown>`);

const element = await page.find("calcite-dropdown");
expect(element).toHaveAttribute(HYDRATED_ATTR);
});

it("renders default props when none are provided", async () => {
const page = await newE2EPage();
await page.setContent(`
Expand Down
9 changes: 4 additions & 5 deletions src/components/calcite-inline-editable/calcite-inline-editable.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { accessible, HYDRATED_ATTR, labelable } from "../../tests/commonTests";
import { accessible, labelable, renders } from "../../tests/commonTests";
import { E2EPage } from "@stencil/core/testing";
import { newE2EPage } from "@stencil/core/testing";
import { CSS } from "./resources";
import { html } from "../../tests/utils";

describe("calcite-inline-editable", () => {
it("renders", () => renders("calcite-inline-editable", { display: "block" }));

describe("rendering permutations", () => {
let page: E2EPage;
beforeEach(async () => {
Expand All @@ -16,10 +18,7 @@ describe("calcite-inline-editable", () => {
`);
});

it("renders", async () => {
const element = await page.find("calcite-inline-editable");
expect(element).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () => renders("calcite-inline-editable", { display: "inline-block" }));

it("renders default props when none are provided", async () => {
const element = await page.find("calcite-inline-editable");
Expand Down
4 changes: 4 additions & 0 deletions src/components/calcite-inline-editable/calcite-inline-editable.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
:host {
@apply block;
Copy link
Member Author

Choose a reason for hiding this comment

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

@eriklharper is this correct? seems input is block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would advocate inline-block for this too.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I change this to inline-block the screener tests will change due to width differences. I will leave as now. If you think it should be changed I think a different issue is needed.

}

:host([scale="s"]) {
.controls-wrapper {
@apply h-6;
Expand Down
9 changes: 2 additions & 7 deletions src/components/calcite-input/calcite-input.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { newE2EPage } from "@stencil/core/testing";
import { focusable, formAssociated, HYDRATED_ATTR, labelable } from "../../tests/commonTests";
import { focusable, formAssociated, renders, labelable } from "../../tests/commonTests";
import { html } from "../../tests/utils";
import { letterKeys, numberKeys } from "../../utils/key";
import { getDecimalSeparator, locales, localizeNumberString } from "../../utils/locale";
Expand Down Expand Up @@ -44,12 +44,7 @@ describe("calcite-input", () => {

it("is labelable", async () => labelable("calcite-input"));

it("renders", async () => {
const page = await newE2EPage();
await page.setContent("<calcite-input></calcite-input>");
const input = await page.find("calcite-input");
expect(input).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () => renders("calcite-input", { display: "block" }));

it("renders default props when none are provided", async () => {
const page = await newE2EPage();
Expand Down
4 changes: 4 additions & 0 deletions src/components/calcite-input/calcite-input.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
:host {
@apply block;
Copy link
Member Author

Choose a reason for hiding this comment

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

@eriklharper what display should this component have? Should it be "block"? Seems like its already block now but I'm not sure why.

Copy link
Member

Choose a reason for hiding this comment

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

cc @macandcheese

Copy link
Contributor

Choose a reason for hiding this comment

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

inline-block is what the native input does, so that's probably the more appropriate one to use for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I change this to inline-block the screener tests will change due to width differences. I will leave as now. If you think it should be changed I think a different issue is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree that both of these should probably be inline-block like their native counterparts. However, I didn't want to break screener tests and user apps. So this would likely be a breaking change.

}

// scales
:host([scale="s"]) {
& input,
Expand Down
9 changes: 2 additions & 7 deletions src/components/calcite-label/calcite-label.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
import { newE2EPage } from "@stencil/core/testing";
import { HYDRATED_ATTR } from "../../tests/commonTests";
import { renders } from "../../tests/commonTests";

describe("calcite-label", () => {
it("renders", async () => {
const page = await newE2EPage();
await page.setContent("<calcite-label></calcite-label>");
const label = await page.find("calcite-label");
expect(label).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () => renders("calcite-label", { display: "inline" }));

it("renders default props when none are provided", async () => {
const page = await newE2EPage();
Expand Down
4 changes: 4 additions & 0 deletions src/components/calcite-label/calcite-label.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
:host {
@apply inline;
driskull marked this conversation as resolved.
Show resolved Hide resolved
}

:host([alignment="start"]) {
text-align: start;
}
Expand Down
19 changes: 3 additions & 16 deletions src/components/calcite-loader/calcite-loader.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,10 @@
import { newE2EPage } from "@stencil/core/testing";
import { HYDRATED_ATTR } from "../../tests/commonTests";
import { renders } from "../../tests/commonTests";

describe("calcite-loader", () => {
it("renders", async () => {
const page = await newE2EPage();
await page.setContent("<calcite-loader></calcite-loader>");
const loader = await page.find("calcite-loader");
expect(loader).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () => renders("calcite-loader", { display: "none", visible: false }));

it("becomes visible when active prop is set", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-loader></calcite-loader>`);
const loader = await page.find("calcite-loader");
expect(await loader.isVisible()).not.toBe(true);
loader.setProperty("active", true);
await page.waitForChanges();
expect(await loader.isVisible()).toBe(true);
});
it("renders: active", () => renders(`<calcite-loader active></calcite-loader>`, { display: "flex", visible: true }));

it("displays label from text prop", async () => {
const page = await newE2EPage();
Expand Down
4 changes: 4 additions & 0 deletions src/components/calcite-loader/calcite-loader.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ $loader-circumference: ($loader-scale - (2 * $stroke-width)) * 3.14159;
@apply py-0;
}

:host {
@apply hidden;
Copy link
Member Author

Choose a reason for hiding this comment

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

loader should be hidden by default and displayed when active.

Copy link
Member

Choose a reason for hiding this comment

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

Sort of a separate discussion, but maybe we should use hidden for this? Looks like it's only used for visibility. cc @macandcheese

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a separate discussion but by default the loader is not shown until it has an active attribute/property. We can evaluate changing this so that the active property is not necessary and just remove it.

Copy link
Member

Choose a reason for hiding this comment

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

#3821

}

:host([active]) {
@apply flex;
}
Expand Down
9 changes: 2 additions & 7 deletions src/components/calcite-modal/calcite-modal.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
import { newE2EPage } from "@stencil/core/testing";
import { focusable, HYDRATED_ATTR } from "../../tests/commonTests";
import { focusable, renders } from "../../tests/commonTests";
import { html } from "../../tests/utils";

describe("calcite-modal properties", () => {
it("renders", async () => {
const page = await newE2EPage();
await page.setContent("<calcite-modal></calcite-modal>");
const element = await page.find("calcite-modal");
expect(element).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () => renders("calcite-modal", { display: "flex" }));

it("adds localized strings set via intl-* props", async () => {
const page = await newE2EPage();
Expand Down
11 changes: 2 additions & 9 deletions src/components/calcite-progress/calcite-progress.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
import { newE2EPage } from "@stencil/core/testing";
import { accessible } from "../../tests/commonTests";
import { accessible, renders } from "../../tests/commonTests";

describe("calcite-progress", () => {
it("renders", async () => {
const page = await newE2EPage();

await page.setContent("<calcite-progress></calcite-progress>");
const element = await page.find("calcite-progress");
expect(element).toBeDefined();
});
it("renders", () => renders("calcite-progress", { display: "block" }));

it("is accessible", async () => accessible(`<calcite-progress label="my progress"></calcite-progress>`));

Expand Down
9 changes: 2 additions & 7 deletions src/components/calcite-radio-group-item/calcite-radio-group-item.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
import { newE2EPage } from "@stencil/core/testing";
import { renders } from "../../tests/commonTests";

describe("calcite-radio-group-item", () => {
it("renders", async () => {
const page = await newE2EPage();
await page.setContent("<calcite-radio-group-item></calcite-radio-group-item>");
const element = await page.find("calcite-radio-group-item");

expect(element).toBeDefined();
});
it("renders", () => renders("calcite-radio-group-item", { display: "flex" }));

it("is un-checked by default", async () => {
const page = await newE2EPage();
Expand Down
10 changes: 2 additions & 8 deletions src/components/calcite-radio-group/calcite-radio-group.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,9 @@
import { E2EPage, newE2EPage } from "@stencil/core/testing";
import { focusable, formAssociated, labelable } from "../../tests/commonTests";
import { focusable, formAssociated, labelable, renders } from "../../tests/commonTests";
import { html } from "../../tests/utils";

describe("calcite-radio-group", () => {
it("renders", async () => {
const page = await newE2EPage();
await page.setContent("<calcite-radio-group></calcite-radio-group>");
const element = await page.find("calcite-radio-group");

expect(element).toBeDefined();
});
it("renders", () => renders("calcite-radio-group", { display: "flex" }));

it("is labelable", async () =>
labelable(
Expand Down
11 changes: 2 additions & 9 deletions src/components/calcite-split-button/calcite-split-button.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { newE2EPage } from "@stencil/core/testing";
import { accessible, HYDRATED_ATTR, defaults } from "../../tests/commonTests";
import { accessible, renders, defaults } from "../../tests/commonTests";
import { CSS } from "./resources";

describe("calcite-split-button", () => {
Expand All @@ -17,14 +17,7 @@ describe("calcite-split-button", () => {
<calcite-dropdown-item id="item-2" active>Item2</calcite-dropdown-item>
</calcite-dropdown-group>`;

it("renders", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-split-button>
</calcite-split-button>`);
const element = await page.find("calcite-split-button");
expect(element).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () => renders("calcite-split-button", { display: "inline-block" }));

it("is accessible", async () =>
accessible(`<calcite-split-button
Expand Down
4 changes: 4 additions & 0 deletions src/components/calcite-split-button/calcite-split-button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
:host {
@apply inline-block w-auto align-middle;
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied this from calcite-button. is this correct @eriklharper?

Copy link
Member

Choose a reason for hiding this comment

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

Should the non-display styles be set on the container instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks right since the native <button> is also inline-block by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the non-display styles be set on the container instead?

Good question. I think the display should probably be set on the host element, but as far as the other properties, there's probably a lot you can move to a container div, unless the host needs like a position property or some other parent-child css property that enables styles to work for elements underneath the host element, like flex for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove the width and align stuff.

}

:host {
.split-button__container {
@apply flex items-stretch;
Expand Down
11 changes: 2 additions & 9 deletions src/components/calcite-stepper-item/calcite-stepper-item.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
import { newE2EPage } from "@stencil/core/testing";
import { HYDRATED_ATTR } from "../../tests/commonTests";
import { renders } from "../../tests/commonTests";

describe("calcite-stepper-item", () => {
it("renders", async () => {
const page = await newE2EPage();

await page.setContent("<calcite-stepper-item></calcite-stepper-item>");
const element = await page.find("calcite-stepper-item");
expect(element).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () => renders("calcite-stepper-item", { display: "flex" }));
});
41 changes: 20 additions & 21 deletions src/components/calcite-stepper/calcite-stepper.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,27 @@
import { newE2EPage } from "@stencil/core/testing";
import { HYDRATED_ATTR } from "../../tests/commonTests";
import { renders } from "../../tests/commonTests";

// todo test the automatic setting of first item to active
describe("calcite-stepper", () => {
it("renders", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-stepper>
<calcite-stepper-item item-title="Step 1" id="step-1">
<div>Step 1 content</div>
</calcite-stepper-item>
<calcite-stepper-item item-title="Step 2" id="step-2">
<div>Step 2 content</div>
</calcite-stepper-item>
<calcite-stepper-item item-title="Step 3" id="step-3">
<div>Step 3 content</div>
</calcite-stepper-item>
<calcite-stepper-item item-title="Step 4" id="step-4">
<div>Step 4 content</div>
</calcite-stepper-item>
</calcite-stepper>`);
const element = await page.find("calcite-stepper");
expect(element).toHaveAttribute(HYDRATED_ATTR);
});
it("renders", () =>
renders(
`
<calcite-stepper>
<calcite-stepper-item item-title="Step 1" id="step-1">
<div>Step 1 content</div>
</calcite-stepper-item>
<calcite-stepper-item item-title="Step 2" id="step-2">
<div>Step 2 content</div>
</calcite-stepper-item>
<calcite-stepper-item item-title="Step 3" id="step-3">
<div>Step 3 content</div>
</calcite-stepper-item>
<calcite-stepper-item item-title="Step 4" id="step-4">
<div>Step 4 content</div>
</calcite-stepper-item>
</calcite-stepper>`,
{ display: "flex" }
));

it("renders default props when none are provided", async () => {
const page = await newE2EPage();
Expand Down
20 changes: 4 additions & 16 deletions src/components/calcite-tab/calcite-tab.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,12 @@
import { newE2EPage } from "@stencil/core/testing";
import { defaults, HYDRATED_ATTR } from "../../tests/commonTests";
import { defaults, renders } from "../../tests/commonTests";

describe("calcite-tab", () => {
const tabHtml = "<calcite-tab>A tab</calcite-tab>";

it("renders", async () => {
const page = await newE2EPage({ html: tabHtml });
const element = await page.find("calcite-tab");
const styles = await element.getComputedStyle();
expect(element).toHaveAttribute(HYDRATED_ATTR);
expect(styles["display"]).toEqual("none");
expect(styles["visibility"]).toEqual("visible");
});
it("renders", () => renders("calcite-tab", { display: "none", visible: false }));

it("renders: active", () => renders("<calcite-tab active></calcite-tab>", { display: "block", visible: true }));
Copy link
Member

Choose a reason for hiding this comment

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

Let's try to group related tests whenever possible w/ describe.


it("has defaults", async () =>
defaults("calcite-tab", [
Expand All @@ -20,13 +15,6 @@ describe("calcite-tab", () => {
{ propertyName: "scale", defaultValue: undefined }
]));

it("has block display when active", async () => {
const page = await newE2EPage({ html: "<calcite-tab active></calcite-tab>" });
const element = await page.find("calcite-tab");
const styles = await element.getComputedStyle();
expect(styles["display"]).toEqual("block");
});

describe("when nested within calcite-tabs component", () => {
it("should render with medium scale", async () => {
const page = await newE2EPage({
Expand Down
Loading