refactor(components): reduce post-migration TypeScript errors #10356

maxpatiiuk · 2024-09-19T17:49:25Z

Minor refactor - some HTML attributes are reflected to properties.
Thus, we can access properties instead of calling getAttribute/hasAttribute.
Properties are also more type-safe (where as getAttribute string parameter does not have type-checking)

And other minor changes.

maxpatiiuk · 2024-09-19T17:49:43Z

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

@@ -764,10 +761,6 @@ export class Dialog
    }
  }

-  private setContainerEl = (el: HTMLDivElement): void => {


Never read, thus removed

maxpatiiuk · 2024-09-19T17:50:03Z

packages/calcite-components/src/components/input-date-picker/input-date-picker.tsx

@@ -774,9 +774,9 @@ export class InputDatePicker

  @State() private localeData: DateLocaleData;

-  private startInput: HTMLCalciteInputElement;
+  private startInput: HTMLCalciteInputTextElement;


This was causing a type error in Lumina - the ref is assigned to calcite-input-text, but the type here was calcite-input.

packages/calcite-components/src/components/input-number/input-number.tsx

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

maxpatiiuk · 2024-09-19T18:20:10Z

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

@@ -146,8 +146,6 @@ export class Tooltip implements FloatingUIComponent, OpenCloseComponent {

  guid = `calcite-tooltip-${guid()}`;

-  hasLoaded = false;


maxpatiiuk · 2024-09-19T18:20:16Z

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

@@ -228,7 +228,7 @@ export class CalciteMenu implements LocalizedComponent, T9nComponent, LoadableCo
  }

  private getEffectiveRole(): string {
-    return this.el.getAttribute("role") || "menubar";


jcfranco · 2024-09-23T17:03:18Z

I think Firefox ESR prevents us from using ARIA-related properties (e.g., ariaExpanded was added in FF 119).

@dasa does this seem accurate to you?

Sidebar: noticed value-list (deprecated) currently uses ariaLabel.

dasa · 2024-09-23T17:20:23Z

We're planning to only support ESR 128 in our next release: https://whattrainisitnow.com/release/?version=esr

jcfranco

Thanks for the PR, @maxpatiiuk!

Along the same lines of #10352, since some additional refactors were made to files unrelated to simplifying reflected attribute accesses, can you either:

separate non-Host-related refactors to a separate PR?
update the PR title to convey all PR changes?

Thanks!

jcfranco · 2024-09-23T18:05:03Z

Thanks for chiming in, @dasa!

maxpatiiuk · 2024-09-24T19:13:50Z

I think Firefox ESR prevents us from using ARIA-related properties (e.g., ariaExpanded was added in FF 119).

We're planning to only support ESR 128 in our next release: https://whattrainisitnow.com/release/?version=esr

@jcfranco so if the next release of JS API will drop Firefox 115 ESR support, is Calcite going to do the same for Calcite 3.0 release?
If so, are we ok to use aria properties in this PR or should I revert the change?

`Tag` and `childElType` are the same in Stencil, but will be different in Lit - so we should use `childElType` in places where the string check is needed.

maxpatiiuk · 2024-09-24T19:22:17Z

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

@@ -17,7 +17,7 @@ export const Heading: FunctionalComponent<HeadingProps> = (props, children): VNo
  delete props.level;

  return (
-    <HeadingTag class={props.class} key={props.key} level={props.level}>


level prop doesn't seem to exist in the DOM?
was it used for styling or testing? if not, we should remove it

original discussion in #10352 (comment)

maxpatiiuk · 2024-09-24T19:22:43Z

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

@@ -114,15 +114,19 @@ export class Link implements InteractiveComponent, LoadableComponent {
          This works around that issue for now.
          */
            download={
-              Tag === "a" ? (download === true || download === "" ? "" : download || null) : null
+              childElType === "a"


As per line 103 in this file (const Tag = childElType;), Tag and childElType variables have the same value in Stencil - thus this change should have no runtime impact.
However, in Lit, Tag function has a bit different type to comply with Lit's dynamic tag name syntax.

See Lumina's docs on JSX -> Dynamic tag names for more details

jcfranco · 2024-09-24T22:48:14Z

If so, are we ok to use aria properties in this PR or should I revert the change?

Sorry for not clarifying earlier. We should be good to use ARIA properties.

jcfranco

Awesome stuff, @maxpatiiuk! Thanks! 🚀

…tracking * origin/dev: (40 commits) feat: add parcel parameter (#10384) feat(alert): apply --calcite-alert-corner-radius to internal close button (#10388) feat(dialog, panel): Add css properties for background-color (#10387) fix: remove aria-disabled from components where necessary (#10374) feat(action-group, block, panel): add `menuPlacement` and `menuFlipPlacements` properties (#10249) fix(list, filter): fix sync between list items and filtered data (#10342) feat(popover): apply component tokens to arrow (#10386) feat(list-item): add `unavailable` property (#10377) fix(segmented-control): honor appearance, layout and scale when items are added after initialization (#10368) fix(action-pad): fix horizontal action group alignment (#10359) fix(combobox): selection-mode="single-persist" correctly selects an item when items have same values (#10366) fix(input-time-zone): fix region mode labeling and value mapping (#10345) fix(dropdown): open dropdown on `ArrowDown` & `ArrowUp` keys (#10264) refactor(components): reduce post-migration TypeScript errors (#10356) refactor: do not use Host in functional components (#10352) refactor(tests): reduce TypeScript errors (#10344) feat(alert): add component tokens (#10218) fix(card): properly handle slotted elements (#10378) refactor(panel): remove duplicate tailwind class (#10379) feat(popover, action): add component tokens (#10253) ...

…estone-estimates * origin/dev: (29 commits) fix(input-time-zone): fix region mode labeling and value mapping (#10345) fix(dropdown): open dropdown on `ArrowDown` & `ArrowUp` keys (#10264) refactor(components): reduce post-migration TypeScript errors (#10356) refactor: do not use Host in functional components (#10352) refactor(tests): reduce TypeScript errors (#10344) feat(alert): add component tokens (#10218) fix(card): properly handle slotted elements (#10378) refactor(panel): remove duplicate tailwind class (#10379) feat(popover, action): add component tokens (#10253) chore(t9n): add translations (#10339) feat: add 3d building, 3d building parameter, divide, parcel, spaces, spaces parameter, square brackets x, n variable, zoning parameter (#10373) build: update browserslist db (#10370) ci: resolve husky pre-push and pre-commit errors (#10365) docs: update component READMEs (#10371) feat(text-area): add component tokens (#10343) fix(action): prefer `disabled` in favor of `aria-disabled` (#10367) docs(a11y): add reference to a11y guidance to issue template (#10372) chore(action): add new string for accessible indicator label (#10360) feat(chip): add component tokens (#10179) feat(avatar): add component tokens (#10280) ...

github-actions bot assigned maxpatiiuk Sep 19, 2024

github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Sep 19, 2024

maxpatiiuk commented Sep 19, 2024

View reviewed changes

jcfranco requested changes Sep 23, 2024

View reviewed changes

maxpatiiuk changed the title ~~refactor: simplify reflected attribute accesses~~ refactor(components): reduce post-migration TypeScript errors Sep 24, 2024

maxpatiiuk added 4 commits September 24, 2024 12:20

refactor: simplify reflected attribute accesses

05e947c

refactor: use correct input-text type

9e64d87

refactor(link): don't conflate JSX tag name and string

5696e22

`Tag` and `childElType` are the same in Stencil, but will be different in Lit - so we should use `childElType` in places where the string check is needed.

fix(Heading): remove non-existent dom prop

a73a79d

maxpatiiuk force-pushed the max/simplify-reflected-attributes branch from 2305e01 to a73a79d Compare September 24, 2024 19:21

maxpatiiuk commented Sep 24, 2024

View reviewed changes

maxpatiiuk mentioned this pull request Sep 24, 2024

refactor: do not use Host in functional components #10352

Merged

maxpatiiuk requested a review from jcfranco September 24, 2024 20:43

jcfranco approved these changes Sep 24, 2024

View reviewed changes

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

jcfranco merged commit 97d8bb8 into dev Sep 24, 2024
15 checks passed

jcfranco deleted the max/simplify-reflected-attributes branch September 24, 2024 23:16

github-actions bot added this to the 2024-09-30 - 2.13.0 Sep Main release milestone Sep 24, 2024

jcfranco mentioned this pull request Sep 24, 2024

Migrate component framework to Lit #10310

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(components): reduce post-migration TypeScript errors #10356

refactor(components): reduce post-migration TypeScript errors #10356

maxpatiiuk commented Sep 19, 2024 •

edited

Loading

maxpatiiuk Sep 19, 2024

maxpatiiuk Sep 19, 2024

maxpatiiuk Sep 19, 2024

maxpatiiuk Sep 19, 2024

jcfranco commented Sep 23, 2024

dasa commented Sep 23, 2024

jcfranco left a comment

jcfranco commented Sep 23, 2024

maxpatiiuk commented Sep 24, 2024

maxpatiiuk Sep 24, 2024

maxpatiiuk Sep 24, 2024

jcfranco commented Sep 24, 2024 •

edited

Loading

jcfranco left a comment

		@@ -146,8 +146,6 @@ export class Tooltip implements FloatingUIComponent, OpenCloseComponent {

		guid = `calcite-tooltip-${guid()}`;

		hasLoaded = false;

refactor(components): reduce post-migration TypeScript errors #10356

refactor(components): reduce post-migration TypeScript errors #10356

Conversation

maxpatiiuk commented Sep 19, 2024 • edited Loading

maxpatiiuk Sep 19, 2024

Choose a reason for hiding this comment

maxpatiiuk Sep 19, 2024

Choose a reason for hiding this comment

maxpatiiuk Sep 19, 2024

Choose a reason for hiding this comment

maxpatiiuk Sep 19, 2024

Choose a reason for hiding this comment

jcfranco commented Sep 23, 2024

dasa commented Sep 23, 2024

jcfranco left a comment

Choose a reason for hiding this comment

jcfranco commented Sep 23, 2024

maxpatiiuk commented Sep 24, 2024

maxpatiiuk Sep 24, 2024

Choose a reason for hiding this comment

maxpatiiuk Sep 24, 2024

Choose a reason for hiding this comment

jcfranco commented Sep 24, 2024 • edited Loading

jcfranco left a comment

Choose a reason for hiding this comment

maxpatiiuk commented Sep 19, 2024 •

edited

Loading

jcfranco commented Sep 24, 2024 •

edited

Loading