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

driskull · 2021-12-22T23:46:06Z

Related Issue: N/A

Summary

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

Uses renders common test util in place of one off renders tests.
Adds default display values where necessary. These will need to be verified.

…isplay.

driskull

Questions

src/components/calcite-label/calcite-label.scss

driskull · 2021-12-23T00:08:14Z

src/components/calcite-loader/calcite-loader.scss

@@ -45,6 +45,10 @@ $loader-circumference: ($loader-scale - (2 * $stroke-width)) * 3.14159;
  @apply py-0;
 }

+:host {
+  @apply hidden;


loader should be hidden by default and displayed when active.

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

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.

driskull · 2021-12-23T00:08:42Z

src/components/calcite-split-button/calcite-split-button.scss

@@ -1,3 +1,7 @@
+:host {
+  @apply inline-block w-auto align-middle;


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

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

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

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.

I'll remove the width and align stuff.

driskull · 2021-12-23T00:08:52Z

src/components/calcite-input/calcite-input.scss

@@ -1,3 +1,7 @@
+:host {
+  @apply block;


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

cc @macandcheese

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

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.

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.

driskull · 2021-12-23T00:09:39Z

src/components/calcite-inline-editable/calcite-inline-editable.scss

@@ -1,3 +1,7 @@
+:host {
+  @apply block;


@eriklharper is this correct? seems input is block.

I would advocate inline-block for this too.

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.

jcfranco

This is looking great. Thanks for setting this up.

jcfranco · 2021-12-23T17:35:49Z

src/components/calcite-split-button/calcite-split-button.scss

@@ -1,3 +1,7 @@
+:host {
+  @apply inline-block w-auto align-middle;


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

jcfranco · 2021-12-23T17:37:04Z

src/components/calcite-tab/calcite-tab.e2e.ts

-  });
+  it("renders", () => renders("calcite-tab", { display: "none", visible: false }));
+
+  it("renders: active", () => renders("<calcite-tab active></calcite-tab>", { display: "block", visible: true }));


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

jcfranco · 2021-12-23T17:38:03Z

src/components/calcite-inline-editable/calcite-inline-editable.e2e.ts

 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(
+      `


Can you use the html helper on multi-line HTML for formatting?

jcfranco · 2021-12-23T17:39:10Z

src/components/calcite-loader/calcite-loader.scss

@@ -45,6 +45,10 @@ $loader-circumference: ($loader-scale - (2 * $stroke-width)) * 3.14159;
  @apply py-0;
 }

+:host {
+  @apply hidden;


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

jcfranco · 2021-12-23T17:54:02Z

src/components/calcite-input/calcite-input.scss

@@ -1,3 +1,7 @@
+:host {
+  @apply block;


cc @macandcheese

driskull · 2021-12-29T20:05:15Z

@jcfranco can you review?

jcfranco

LGTM!

@eriklharper @macandcheese I second @driskull's suggestion to open a new issue if any of the defaults need to change.

jcfranco · 2021-12-30T19:26:02Z

src/tests/commonTests.ts

@@ -60,7 +60,7 @@ export async function renders(
  componentTagOrHTML: TagOrHTML,
  options?: {
    visible?: boolean;
-    display?: string;
+    display: string;


jcfranco · 2021-12-30T19:34:35Z

src/components/calcite-loader/calcite-loader.scss

@@ -45,6 +45,10 @@ $loader-circumference: ($loader-scale - (2 * $stroke-width)) * 3.14159;
  @apply py-0;
 }

+:host {
+  @apply hidden;


refactor: Update tests to use common test and apply correct default d…

d2e49ee

…isplay.

github-actions bot added this to the Sprint 12/20 - 12/31 milestone Dec 22, 2021

github-actions bot assigned driskull Dec 22, 2021

github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Dec 22, 2021

driskull added 2 commits December 22, 2021 16:03

fixes

a59fbd2

fixes

3e119e3

driskull commented Dec 23, 2021

View reviewed changes

driskull requested review from jcfranco and eriklharper December 23, 2021 00:17

driskull marked this pull request as ready for review December 23, 2021 00:18

driskull requested a review from a team as a code owner December 23, 2021 00:18

driskull added 3 commits December 22, 2021 16:26

fix some tests

11cf67a

fix test

8cbdf52

make display option required

03bcdac

jcfranco reviewed Dec 23, 2021

View reviewed changes

review fixes

68e3554

driskull requested a review from jcfranco December 27, 2021 18:19

Merge branch 'master' into dris0000/default-display-tests

0902327

Merge branch 'master' into dris0000/default-display-tests

7d9ed8e

jcfranco approved these changes Dec 30, 2021

View reviewed changes

driskull merged commit e4ee0a5 into master Dec 30, 2021

driskull deleted the dris0000/default-display-tests branch December 30, 2021 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

driskull commented Dec 22, 2021

driskull left a comment

driskull Dec 23, 2021

jcfranco Dec 23, 2021

driskull Dec 27, 2021

jcfranco Dec 30, 2021

driskull Dec 23, 2021

jcfranco Dec 23, 2021

eriklharper Dec 23, 2021

eriklharper Dec 23, 2021

driskull Dec 27, 2021

driskull Dec 23, 2021

jcfranco Dec 23, 2021

eriklharper Dec 23, 2021

driskull Dec 27, 2021

driskull Dec 30, 2021

driskull Dec 23, 2021

eriklharper Dec 23, 2021

driskull Dec 27, 2021

jcfranco left a comment

jcfranco Dec 23, 2021

jcfranco Dec 23, 2021

jcfranco Dec 23, 2021

jcfranco Dec 23, 2021

jcfranco Dec 23, 2021

driskull commented Dec 29, 2021

jcfranco left a comment

jcfranco Dec 30, 2021

jcfranco Dec 30, 2021

		@@ -1,3 +1,7 @@
		:host {
		@apply inline-block w-auto align-middle;

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

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

Conversation

driskull commented Dec 22, 2021

Summary

driskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driskull commented Dec 29, 2021

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment