Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

fix(shell-center-row): Error when no action-bar is provided #1032

Merged
merged 8 commits into from
Jul 10, 2020
Merged

fix(shell-center-row): Error when no action-bar is provided #1032

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f9fb8bf
#1031 fix shell center row error
driskull Jul 10, 2020
90d5d6a
Merge branch 'master' into dris0000/fix-shell-center-row
driskull Jul 10, 2020
446b2a0
typing
driskull Jul 10, 2020
aa4ece3
add test?
driskull Jul 10, 2020
5085e8c
cleanup
driskull Jul 10, 2020
6806da7
cleanup
driskull Jul 10, 2020
a461e95
rework tests
driskull Jul 10, 2020
13a8f25
Merge branch 'master' into dris0000/fix-shell-center-row
driskull Jul 10, 2020
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
13 changes: 13 additions & 0 deletions src/components/calcite-shell-center-row/calcite-shell-center-row.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,19 @@ describe("calcite-shell-center-row", () => {
}
]));

it("should not error when there is no action-bar", async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@jcfranco could we make this test reusable like the renders test? I don't think any components should throw a console.error.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like there's already a way to fail on console error:

it("should not error when there is no action-bar", async () =>
  await newE2EPage({
    html: "<calcite-shell-center-row></calcite-shell-center-row>",
    failOnConsoleError: true
  }));

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh nice. Ill test it out. Can we turn it on by default?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... I don't see a way to enable it globally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we maybe add this to the renders test?

const page = await newE2EPage();

const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation();

await page.setContent("<calcite-shell-center-row></calcite-shell-center-row>");

expect(console.error).toHaveBeenCalledTimes(0);

consoleErrorSpy.mockClear();
consoleErrorSpy.mockRestore();
});

it("should not render action bar container when there is no action-bar", async () => {
const page = await newE2EPage();

Expand Down
6 changes: 3 additions & 3 deletions src/components/calcite-shell-center-row/calcite-shell-center-row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ export class CalciteShellCenterRow {
</div>
) : null;

const children = [actionBarNode, contentNode];
if (actionBar.position === "end") {
const children: VNode[] = [actionBarNode, contentNode];

if (actionBar?.position === "end") {
children.reverse();
}

Expand Down