Skip to content

Commit

Permalink
fix(tree): fix item selection when multi + input-enabled (#2555)
Browse files Browse the repository at this point in the history
  • Loading branch information
jcfranco committed Jul 16, 2021
1 parent 45de765 commit 44af309
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/components/calcite-tree-item/calcite-tree-item.tsx
Expand Up @@ -191,7 +191,7 @@ export class CalciteTreeItem {
}
this.expanded = !this.expanded;
this.calciteTreeItemSelect.emit({
modifyCurrentSelection: (e as any).shiftKey,
modifyCurrentSelection: (e as any).shiftKey || this.inputEnabled,
forceToggle: false
});
}
Expand Down
41 changes: 41 additions & 0 deletions src/components/calcite-tree/calcite-tree.e2e.ts
Expand Up @@ -187,6 +187,47 @@ describe("calcite-tree", () => {
expect(selectEventSpy).toHaveReceivedEventTimes(3);
});

describe("has selected items in the selection event payload", () =>

This comment has been minimized.

Copy link
@driskull

driskull Jul 21, 2021

Member

@jcfranco FYI this is throwing a test warning because describe can't return a value.

console.log
        ● Test suite failed to run
      
          A "describe" callback must not return a value.
          Returning a value from "describe" will fail the test in a future version of Jest.
      
            188 |     });
            189 | 
          > 190 |     describe("has selected items in the selection event payload", () =>
                |     ^
            191 |       it("contains current selection when selection=multi + input-enabled", async () => {
            192 |         const page = await newE2EPage({
            193 |           html: html` <calcite-tree selection-mode="multi" input-enabled>
      
            at addSpecsToSuite (node_modules/jest-jasmine2/build/jasmine/Env.js:470:17)
            at Suite.<anonymous> (src/components/calcite-tree/calcite-tree.e2e.ts:190:5)
            at Suite.<anonymous> (src/components/calcite-tree/calcite-tree.e2e.ts:152:3)
      at addSpecsToSuite (node_modules/jest-jasmine2/build/jasmine/Env.js:468:21)

I think this is why we should clean up our warnings in the build so we can spot these easier and prevent necessary fixes in the future.

This comment has been minimized.

Copy link
@jcfranco

jcfranco Jul 21, 2021

Author Member

Total oversight on my part. I was only looking at the test results. 😅 I'll fix the warning shortly.

I thought the linter could help catch this, but unfortunately, it looks like jest/valid-describe doesn't report on this. I've submitted a PR jest-community/eslint-plugin-jest#863 in the meantime.

This comment has been minimized.

Copy link
@jcfranco

jcfranco Jul 21, 2021

Author Member

My PR got merged and shipped! 🎉

#2585 will help catch value-returning describe blocks. 💪🏼

it("contains current selection when selection=multi + input-enabled", async () => {
const page = await newE2EPage({
html: html` <calcite-tree selection-mode="multi" input-enabled>
<calcite-tree-item id="1">1</calcite-tree-item>
<calcite-tree-item id="2">2</calcite-tree-item>
</calcite-tree>`
});

const [item1, item2] = await page.findAll("calcite-tree-item");

type TestWindow = {
selectedIds: string[];
} & Window &
typeof globalThis;

await page.evaluateHandle(() =>
document.addEventListener("calciteTreeSelect", ({ detail }: CustomEvent) => {
(window as TestWindow).selectedIds = detail.selected.map((item) => item.id);
})
);

const getSelectedIds = async (): Promise<any> => page.evaluate(() => (window as TestWindow).selectedIds);

await item1.click();

expect(await getSelectedIds()).toEqual(["1"]);

await item2.click();

expect(await getSelectedIds()).toEqual(["1", "2"]);

await item2.click();

expect(await getSelectedIds()).toEqual(["1"]);

await item1.click();

expect(await getSelectedIds()).toEqual([]);
}));

it("emits once when the tree item checkbox label is clicked", async () => {
const page = await newE2EPage({
html: html`<calcite-tree input-enabled selection-mode="ancestors">
Expand Down

0 comments on commit 44af309

Please sign in to comment.