refactor: fixup typescript errors #10295

maxpatiiuk · 2024-09-13T04:09:56Z

With the changes in this PR, after running the codemod, there are only 60 TypeScript errors in the Calcite package - those should be best fixed manually after running the codemod.
Now that error count is down to negligible level, will work on migrating tests.

maxpatiiuk · 2024-09-13T04:11:54Z

packages/calcite-components/src/components/button/button.tsx

@@ -279,7 +279,13 @@ export class Button
          }}
          disabled={childElType === "button" ? this.disabled || this.loading : null}
          download={
-            childElType === "a" && (this.download === "" || this.download) ? this.download : null
+            childElType === "a"


with this change, when this.download is true, we set the prop as "" instead.
not required for Stencil, but this is needed for Lit as download attribute is typed as string rather than string | boolean in Lumina for technical reasons (it's one few such special attributes for legacy dom reasons)

This needs to be updated or reverted. It breaks this test.

This change changes how the internal element's download attribute is applied.

before

after

maxpatiiuk · 2024-09-13T04:12:24Z

packages/calcite-components/src/components/dialog/dialog.tsx

@@ -43,7 +43,7 @@ import {
 import { componentOnReady } from "../../utils/component";
 import { SLOTS as PANEL_SLOTS } from "../panel/resources";
 import { HeadingLevel } from "../functional/Heading";
-import { OverlayPositioning } from "../../components";
+import type { OverlayPositioning } from "../../utils/floating-ui";


since components.d.ts will be deleted, I'm changing this to import from the actual origin

maxpatiiuk · 2024-09-13T04:12:44Z

packages/calcite-components/src/components/functional/FloatingArrow.tsx

@@ -2,7 +2,7 @@ import { FunctionalComponent, h, VNode } from "@stencil/core";
 import { JSXAttributes } from "@stencil/core/internal";
 import { FloatingLayout } from "../../utils/floating-ui";

-interface FloatingArrowProps extends JSXAttributes {
+interface FloatingArrowProps extends JSXAttributes<SVGSVGElement> {


this way ref callback is typed correctly, rather than as broad Element

maxpatiiuk · 2024-09-13T04:12:58Z

packages/calcite-components/src/components/link/link.tsx

@@ -113,7 +113,7 @@ export class Link implements InteractiveComponent, LoadableComponent {
          When the 'download' property of type 'boolean | string' is set to true, the value is "".
          This works around that issue for now.
          */
-            download={Tag === "a" && (download === "" || download) ? download : null}
+            download={Tag === "a" ? (download === true ? "" : download ? download : null) : null}


same as in button.tsx

Similar to button, this change breaks this test.

maxpatiiuk · 2024-09-13T04:13:10Z

packages/calcite-components/src/components/link/link.tsx

@@ -174,7 +174,7 @@ export class Link implements InteractiveComponent, LoadableComponent {
  /** the rendered child element */
  private childEl: HTMLAnchorElement | HTMLSpanElement;

-  private childElClickHandler = (event: PointerEvent): void => {
+  private childElClickHandler = (event: MouseEvent): void => {


onClick emits MouseEvent, not PointerEvent

FWIW, this might change in the near future since Chrome and Firefox already emit pointer events, and it looks like Safari will soon (adhering to the updated spec).

Thanks for the fyi.
I will revert this change and update Lumina's JSX typings.

For the future, I am looking to automate the process of keeping Lumina's JSX typings up to date with the web platform to fix the DX issue we are having with Maquette and Stencil.

maxpatiiuk · 2024-09-13T04:14:40Z

packages/calcite-components/src/components/popover/popover.tsx

@@ -268,7 +268,7 @@ export class Popover

  @State() defaultMessages: PopoverMessages;

-  arrowEl: SVGElement;
+  arrowEl: SVGSVGElement;


the correct type for the <svg> element is SVGSVGElement.
SVGElement means "any element from the SVG namespace", just like HTMLElement means "any element from the HTML namespace"

maxpatiiuk · 2024-09-13T04:14:52Z

packages/calcite-components/src/components/rating/rating.tsx

@@ -356,7 +356,7 @@ export class Rating
    }
  };

-  private handleInputChange = (event: InputEvent) => {
+  private handleInputChange = (event: Event) => {


onChange emits Event
onInput emits InputEvent

maxpatiiuk · 2024-09-13T04:15:09Z

packages/calcite-components/src/components/tooltip/tooltip.tsx

@@ -344,7 +344,7 @@ export class Tooltip implements FloatingUIComponent, OpenCloseComponent {
        >
          <FloatingArrow
            floatingLayout={floatingLayout}
-            ref={(arrowEl: SVGElement) => (this.arrowEl = arrowEl)}
+            ref={(arrowEl) => (this.arrowEl = arrowEl)}


no longer need to type this here now that FloatingArrow ref type is typed correctly

maxpatiiuk · 2024-09-13T04:15:40Z

packages/calcite-components/src/demos/input-time-picker.html

these link/script tags seem needless here since they are inserted by _assets/head.ts anyway

Thanks for the cleanup!

BTW, I extracted the demo-related changes to a separate PR to keep this one focused on TS errors.

Sounds good. I will undo those changes in this PR

maxpatiiuk · 2024-09-13T04:17:20Z

packages/eslint-plugin-calcite-components/src/rules/require-event-emitter-type.ts

@@ -1,5 +1,4 @@
 import { Rule } from "eslint";
-// @ts-ignore


PSA: do not use // @ts-ignore as it's dangerous.
Reason: once the thing that was causing the TypeScript error is fixed, the // @ts-ignore comment may remain in the code. In the future, if another TypeScript error may occur in that place, you won't see it because // @ts-ignore would hide it.

Safer alternative: // @ts-expect-error - acts just like // @ts-ignore, except, if no error was reported, TypeScript will complain of needless // @ts-expect-error (just like it was needless in this case it seems like)

jcfranco

Looking great, @maxpatiiuk! 🚀

Before moving forward, can you address the button and link-related test failures?

jcfranco · 2024-09-14T01:22:27Z

packages/calcite-components/src/components/button/button.tsx

@@ -279,7 +279,13 @@ export class Button
          }}
          disabled={childElType === "button" ? this.disabled || this.loading : null}
          download={
-            childElType === "a" && (this.download === "" || this.download) ? this.download : null
+            childElType === "a"


This needs to be updated or reverted. It breaks this test.

This change changes how the internal element's download attribute is applied.

before

after

jcfranco · 2024-09-14T01:23:28Z

packages/calcite-components/src/components/link/link.tsx

@@ -113,7 +113,7 @@ export class Link implements InteractiveComponent, LoadableComponent {
          When the 'download' property of type 'boolean | string' is set to true, the value is "".
          This works around that issue for now.
          */
-            download={Tag === "a" && (download === "" || download) ? download : null}
+            download={Tag === "a" ? (download === true ? "" : download ? download : null) : null}


Similar to button, this change breaks this test.

jcfranco · 2024-09-14T01:53:32Z

packages/calcite-components/src/components/link/link.tsx

@@ -174,7 +174,7 @@ export class Link implements InteractiveComponent, LoadableComponent {
  /** the rendered child element */
  private childEl: HTMLAnchorElement | HTMLSpanElement;

-  private childElClickHandler = (event: PointerEvent): void => {
+  private childElClickHandler = (event: MouseEvent): void => {


FWIW, this might change in the near future since Chrome and Firefox already emit pointer events, and it looks like Safari will soon (adhering to the updated spec).

jcfranco · 2024-09-14T02:01:07Z

packages/calcite-components/src/components/handle/handle.e2e.ts

 import { CSS, SUBSTITUTIONS } from "./resources";
+import type { HandleMessages } from "./assets/handle/t9n";


This rearrangement causes an import/order lint error. Can you run the linter to autofix?

If you have a custom groups option for that rule, I can update ours separately to follow suit.

hmm, interesting that pre-commit hook didn't catch this. maybe my node_modules was out of date 🤔

jcfranco · 2024-09-14T05:46:15Z

packages/calcite-components/src/demos/input-time-picker.html

Thanks for the cleanup!

BTW, I extracted the demo-related changes to a separate PR to keep this one focused on TS errors.

maxpatiiuk · 2024-09-15T17:12:29Z

Tooltip e2e test is failing on the CI.
Doesn't seem related to the changes I done in the PR.
Locally, a different tooltip e2e test is failing (calcite-tooltip › openClose › emits with animations enable).
Unstable test, or is there something wrong with my local setup?

jcfranco · 2024-09-15T17:33:08Z

That's an unstable test (I'll try to stabilize soon). I've triggered another run of E2E tests in the meantime.

jcfranco

Awesome! Thanks, @maxpatiiuk! 🚀

**Related Issue:** N/A ## Summary Extracts demo page cleanup from #10295. Co-authored-by: Max Patiiuk <max@patii.uk>

…estone-estimates * origin/dev: (997 commits) fix: correct non-standard filled icon names (#10309) fix(panel): initially closed panel should be hidden (#10308) chore(linting): automate tracking of custom Sass functions for stylelint (#10313) chore: tidy up demo pages (#10314) build(deps): update dependency dayjs to v1.11.13 (#10283) build(deps): update dependency jsdom to v24.1.3 (#10298) build(deps): update dependency husky to v9.1.6 (#10318) build(deps): update angular-cli monorepo to v18.2.4 (#10317) docs: update component READMEs (#10316) refactor(stylelint): change config to module format to enable more dynamic options (#10311) refactor: fixup typescript errors (#10295) build(deps): update dependency lint-staged to v15.2.10 (#10302) build(deps): update dependency focus-trap to v7.6.0 (#10281) build(deps): update dependency husky to v9.1.5 (#10297) chore: release next feat(checkbox): add component tokens (#10221) revert: "chore: set default page size for E2E tests (#10219)" (#10299) chore(icons): ensure UI icons follow naming convention (#10292) chore: release next feat: add model-history, raster-function-history, raster function-template-history, raster-tool-history, tool-history (#10305) ...

maxpatiiuk requested a review from jcfranco as a code owner September 13, 2024 04:09

maxpatiiuk commented Sep 13, 2024

View reviewed changes

jcfranco mentioned this pull request Sep 14, 2024

chore: tidy up demo pages #10314

Merged

jcfranco requested changes Sep 14, 2024

View reviewed changes

maxpatiiuk force-pushed the max/fixup-typescript-errors branch from 460c6e8 to 0d8576e Compare September 15, 2024 16:31

maxpatiiuk added 2 commits September 15, 2024 09:45

refactor: fixup typescript errors

433ddff

fix(button,link): correctly handle empty string download prop

a5a393c

maxpatiiuk force-pushed the max/fixup-typescript-errors branch from 0d8576e to a5a393c Compare September 15, 2024 16:49

maxpatiiuk requested a review from jcfranco September 15, 2024 17:11

jcfranco approved these changes Sep 15, 2024

View reviewed changes

jcfranco added the skip visual snapshots Pull requests that do not need visual regression testing. label Sep 15, 2024

jcfranco merged commit 6a86d80 into Esri:dev Sep 15, 2024
9 of 12 checks passed

jcfranco added a commit that referenced this pull request Sep 16, 2024

chore: tidy up demo pages (#10314)

9afbb48

**Related Issue:** N/A ## Summary Extracts demo page cleanup from #10295. Co-authored-by: Max Patiiuk <max@patii.uk>

jcfranco mentioned this pull request Oct 29, 2024

fix!: floating-ui elements no longer take up space when closed #10241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: fixup typescript errors #10295

refactor: fixup typescript errors #10295

maxpatiiuk commented Sep 13, 2024 •

edited

Loading

maxpatiiuk Sep 13, 2024 •

edited

Loading

jcfranco Sep 14, 2024

maxpatiiuk Sep 13, 2024

maxpatiiuk Sep 13, 2024

maxpatiiuk Sep 13, 2024

jcfranco Sep 14, 2024

maxpatiiuk Sep 13, 2024

jcfranco Sep 14, 2024

maxpatiiuk Sep 14, 2024

maxpatiiuk Sep 13, 2024

maxpatiiuk Sep 13, 2024

maxpatiiuk Sep 13, 2024

maxpatiiuk Sep 13, 2024

jcfranco Sep 14, 2024

maxpatiiuk Sep 15, 2024

maxpatiiuk Sep 13, 2024

jcfranco left a comment

jcfranco Sep 14, 2024

jcfranco Sep 14, 2024

jcfranco Sep 14, 2024

jcfranco Sep 14, 2024

maxpatiiuk Sep 15, 2024

jcfranco Sep 14, 2024

maxpatiiuk commented Sep 15, 2024

jcfranco commented Sep 15, 2024

jcfranco left a comment

		import { CSS, SUBSTITUTIONS } from "./resources";
		import type { HandleMessages } from "./assets/handle/t9n";

refactor: fixup typescript errors #10295

refactor: fixup typescript errors #10295

Conversation

maxpatiiuk commented Sep 13, 2024 • edited Loading

maxpatiiuk Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

before

after

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

before

after

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

maxpatiiuk commented Sep 15, 2024

jcfranco commented Sep 15, 2024

jcfranco left a comment

Choose a reason for hiding this comment

maxpatiiuk commented Sep 13, 2024 •

edited

Loading

maxpatiiuk Sep 13, 2024 •

edited

Loading