Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: do not use Host in functional components #10352

Merged
merged 1 commit into from
Sep 24, 2024
Merged

refactor: do not use Host in functional components #10352

merged 1 commit into from
Sep 24, 2024

Conversation

maxpatiiuk
Copy link
Member

@maxpatiiuk maxpatiiuk commented Sep 19, 2024 •
edited
Loading

Lumina does not have a <Host> element, but there is an equivalent syntax.
The codemod can take care of migrating to the alternative syntax, as long as the <Host> element appears in the render() methods.
This PR moves the <Host> element from the <List> functional component into the parent render() methods.
With this change, all <Host> elements in Calcite's codebase are migrated automatically.

@github-actions github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Sep 19, 2024
maxpatiiuk
maxpatiiuk commented Sep 19, 2024
View reviewed changes
packages/calcite-components/src/components/functional/Heading.tsx Outdated
@@ -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}>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine to remove. @driskull Can you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this change into different PR:
https://github.com/Esri/calcite-design-system/pull/10356/files#diff-59deb5eec2ddeae1c8f1b9c2b630149686c86a41707b01c9ecb949d2c9321867L20

jcfranco
jcfranco requested changes Sep 23, 2024
View reviewed changes
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.

Thanks for the PR, @maxpatiiuk!

Since some additional refactors were made to files unrelated to avoiding using host in functional components, can you either:

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

Thanks!

packages/calcite-components/src/components/functional/Heading.tsx Outdated
@@ -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}>
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine to remove. @driskull Can you confirm?

jcfranco
jcfranco requested changes Sep 23, 2024
View reviewed changes
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.

Thanks for the PR, @maxpatiiuk!

Since some additional refactors were made to files unrelated to avoiding using host in functional components, can you either:

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

Thanks!

@jcfranco jcfranco mentioned this pull request Sep 23, 2024
Merged
@maxpatiiuk
refactor: do not use Host in functional components
53cae99 
Lumina does not have a `<Host>` element, but there is an equivalent
syntax.

The codemod can take care of migrating to the alternative syntax, as
long as the `<Host>` element appears in the `render()` methods.

This PR moves the `<Host>` element from the `<List>` functional
component into the parent `render()` methods.

With this change, all `<Host>` elements in Calcite's codebase are
migrated automatically.
@maxpatiiuk
Copy link
Member Author

Moved non-Host changes into #10356

@maxpatiiuk
Copy link
Member Author

Test failed on the CI but passed locally.
https://github.com/Esri/calcite-design-system/actions/runs/11020374165/job/30605015832
Will force CI to re-run.

Do you commonly experience flakiness with e2e tests? Do you know what are the main sources of flakiness in your tests? Is this something that can be addressed during migration to Vitest browser mode?

jcfranco
jcfranco approved these changes Sep 24, 2024
View reviewed changes
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.

🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊
🌊🌊🦈🦈🦈🌊🦈🦈🦈🦈🌊🦈🌊🌊🌊🌊🌊🦈🌊🌊🦈🦈🦈🌊🌊🦈🦈🌊🌊🦈🌊🌊🌊🦈🌊🦈🦈🦈🦈🌊🦈🌊
🌊🌊🌊🦈🌊🌊🦈🌊🌊🦈🌊🦈🌊🌊🌊🌊🌊🦈🌊🦈🌊🌊🌊🌊🦈🌊🌊🦈🌊🦈🦈🌊🦈🦈🌊🦈🌊🌊🌊🌊🦈🌊
🌊🌊🌊🦈🌊🌊🦈🦈🦈🦈🌊🦈🌊🌊🦈🌊🌊🦈🌊🌊🦈🦈🌊🌊🦈🌊🌊🦈🌊🦈🌊🦈🌊🦈🌊🦈🦈🦈🌊🌊🦈🌊
🌊🦈🌊🦈🌊🌊🦈🌊🌊🦈🌊🦈🌊🦈🌊🦈🌊🦈🌊🌊🌊🌊🦈🌊🦈🌊🌊🦈🌊🦈🌊🌊🌊🦈🌊🦈🌊🌊🌊🌊🌊🌊
🌊🌊🦈🦈🌊🌊🦈🌊🌊🦈🌊🌊🦈🌊🌊🌊🦈🌊🌊🦈🦈🦈🌊🌊🌊🦈🦈🌊🌊🦈🌊🌊🌊🦈🌊🦈🦈🦈🦈🌊🦈🌊
🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊🌊

@jcfranco
Copy link
Member

Do you commonly experience flakiness with e2e tests? Do you know what are the main sources of flakiness in your tests? Is this something that can be addressed during migration to Vitest browser mode?

We have experienced test flakiness. Hard to pinpoint the main source, but here are some culprits:

  1. missing await in tests
  2. environment differences between local machines and CI
  3. non-deterministic tests due to timing (sometimes will surface from ☝️)

We're actively working on stabilizing tests. When we identify a flaky one, we skip it and create a follow-up issue to restore it. By the way, would you mind sharing the flaky test you encountered? We can take a closer look at it.

@jcfranco jcfranco added the skip visual snapshots Pull requests that do not need visual regression testing. label Sep 24, 2024
@jcfranco jcfranco merged commit 427ee46 into dev Sep 24, 2024
14 checks passed
@jcfranco jcfranco deleted the max/no-host-in-functions branch September 24, 2024 23:15
@jcfranco jcfranco mentioned this pull request Sep 24, 2024
Open
@maxpatiiuk
Copy link
Member Author

Thanks for the info!

The flaky test:
src/components/input-number/input-number.e2e.ts > calcite-input-number › supports global attribute casing

Link to error message:
https://github.com/Esri/calcite-design-system/actions/runs/10946245777/job/30392294163

@esri/calcite-components:test:     expect(received).toBe(expected) // Object.is equality
@esri/calcite-components:test: 
@esri/calcite-components:test:     Expected: "tel"
@esri/calcite-components:test:     Received: "decimal"
@esri/calcite-components:test: 
@esri/calcite-components:test:       145 |
@esri/calcite-components:test:       146 |     expect(input.getAttribute("autofocus")).toBe("");
@esri/calcite-components:test:     > 147 |     expect(input.getAttribute("inputmode")).toBe(testInputMode);
@esri/calcite-components:test:           |                                             ^
@esri/calcite-components:test:       148 |     expect(input.getAttribute("enterkeyhint")).toBe(testEnterKeyHint);
@esri/calcite-components:test:       149 |   });
@esri/calcite-components:test:       150 |
@esri/calcite-components:test: 
@esri/calcite-components:test:       at Object.<anonymous> (src/components/input/common/tests.ts:147:45)

When we identify a flaky one, we skip it and create a follow-up issue to restore it

Do you "skip" the test in a separate pull request or in the pull request where the flakiness was identified?

benelan added a commit that referenced this pull request Sep 26, 2024
@benelan
Merge remote-tracking branch 'origin/dev' into benelan/8359-estimate-…
610d68d 
…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)
  ...
@jcfranco
Copy link
Member

Do you "skip" the test in a separate pull request or in the pull request where the flakiness was identified?

Generally, we skip in a separate PR. We have some info on this in our testing conventions.

benelan added a commit that referenced this pull request Sep 30, 2024
@benelan
Merge remote-tracking branch 'origin/dev' into benelan/8359-track-mil…
22a09da 
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

Assignees

@maxpatiiuk maxpatiiuk

Labels
refactor Issues tied to code that needs to be significantly reworked. skip visual snapshots Pull requests that do not need visual regression testing.
Projects
None yet
Milestone
2024-09-30 - 2.13.0 Sep Main release
Development

Successfully merging this pull request may close these issues.
2 participants
@maxpatiiuk @jcfranco