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

Conversation

driskull
Copy link
Member

Related Issue: #1031

Summary

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

@driskull driskull added bug Something isn't working 1 - assigned labels Jul 10, 2020
@driskull driskull added this to the Ocean Princess milestone Jul 10, 2020
@driskull driskull requested a review from asangma July 10, 2020 15:45
@driskull driskull requested a review from a team as a code owner July 10, 2020 15:45
@driskull driskull self-assigned this Jul 10, 2020
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Can you add a test for this before merging?

Copy link
Contributor

@asangma asangma left a comment

Choose a reason for hiding this comment

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

LOL! I Was just about to make a PR for this.

@driskull
Copy link
Member Author

@jcfranco can you review the test? I'm not sure if this is the best way to detect the error but I could not find another solution.

If we do want to go with this, it seems like a good candidate to be something we test for automatically for each component.

@driskull driskull requested a review from jcfranco July 10, 2020 17:10
@@ -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?

Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

👍

@driskull driskull merged commit 050c136 into master Jul 10, 2020
@driskull driskull deleted the dris0000/fix-shell-center-row branch July 10, 2020 22:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 - assigned bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants