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.

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

refactor: create a common resource to store debounce consts #9829

Merged
merged 19 commits into from
Jul 27, 2024

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Jul 22, 2024

Related Issue: #5371

Summary

Refactor: create a common resource to store debounce consts for action-bar, combobox, filter, list, floating-ui.

@Elijbet Elijbet force-pushed the elijbet/5371-common-resource-for-debounce-consts branch from 8c3683c to f97ca1f Compare July 22, 2024 18:28
@github-actions github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Jul 22, 2024
@Elijbet Elijbet marked this pull request as ready for review July 22, 2024 19:39
@Elijbet Elijbet requested review from driskull, jcfranco and benelan and removed request for driskull July 22, 2024 19:40
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.

This is headed in the right direction, but let's try to:

  • move shared debounce delays to calcite-components/src/utils/resources.ts
  • consolidate values by context (e.g., filtering, positioning)

packages/calcite-components/src/utils/debounceValues.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/debounceValues.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/debounceValues.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/debounceValues.ts Outdated Show resolved Hide resolved
packages/calcite-components/src/utils/debounceValues.ts Outdated Show resolved Hide resolved
@driskull
Copy link
Member

Agreed.

Something like:

export const debounceValues = {
  reposition: 100,
  filter: 250,
  overflow: 150,
  nextTick: 0
};

export const debounceValues = {
filter: 250,
nextTick: 0,
overflow: 150,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should rename overflow to resize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in this case it's wrapping the resize method.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Lets rename to resize 👍

Copy link
Member

Choose a reason for hiding this comment

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

I dig it!

@@ -3,7 +3,6 @@ import { SLOTS as ACTION_GROUP_SLOTS } from "../action-group/resources";
import { SLOTS as ACTION_MENU_SLOTS } from "../action-menu/resources";
import { Layout } from "../interfaces";

export const overflowActionsDebounceInMs = 150;
Copy link
Member

Choose a reason for hiding this comment

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

✨🧹✨

packages/calcite-components/src/utils/resources.ts Outdated Show resolved Hide resolved
export const debounceValues = {
filter: 250,
nextTick: 0,
overflow: 150,
Copy link
Member

Choose a reason for hiding this comment

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

I dig it!

@Elijbet Elijbet added the skip visual snapshots Pull requests that do not need visual regression testing. label Jul 24, 2024
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.

🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉
🎉🥳🎉🎉🎉🥳🎉🎉🥳🥳🎉🎉🥳🥳🥳🎉🎉🥳🥳🥳🎉🥳🥳🥳🥳🎉🥳🎉
🎉🥳🥳🎉🎉🥳🎉🥳🎉🎉🥳🎉🎉🥳🎉🎉🥳🎉🎉🎉🎉🥳🎉🎉🎉🎉🥳🎉
🎉🥳🎉🥳🎉🥳🎉🥳🎉🎉🥳🎉🎉🥳🎉🎉🥳🎉🎉🎉🎉🥳🥳🥳🎉🎉🥳🎉
🎉🥳🎉🎉🥳🥳🎉🥳🎉🎉🥳🎉🎉🥳🎉🎉🥳🎉🎉🎉🎉🥳🎉🎉🎉🎉🎉🎉
🎉🥳🎉🎉🎉🥳🎉🎉🥳🥳🎉🎉🥳🥳🥳🎉🎉🥳🥳🥳🎉🥳🥳🥳🥳🎉🥳🎉
🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉

await input.press("Tab");

await page.waitForChanges();
await page.waitForTimeout(DEBOUNCE.filter);
Copy link
Member

Choose a reason for hiding this comment

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

As a follow-up, let's see if this extra pair of waitForXs are needed. Also, some of the E2E/Puppeteer APIs call waitForChanges under the scenes, so some calls may not be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'm removing that waitForX double, seem so to pass without it.

@Elijbet Elijbet merged commit ee894de into dev Jul 27, 2024
9 checks passed
@Elijbet Elijbet deleted the elijbet/5371-common-resource-for-debounce-consts branch July 27, 2024 00:01
@github-actions github-actions bot added this to the 2024-07-30 - Jul Release milestone Jul 27, 2024
benelan added a commit that referenced this pull request Jul 29, 2024
…-to-monorepo

* origin/dev:
  ci: fix env var case in workflows (#9877)
  docs(action): deprecates the compact property (#9847)
  fix(tab-title): Adjust hover styling for `bordered` Tab Title (#9867)
  chore(themed): add token CSS variable test helper (#9860)
  chore: avoid deleting untracked, non-generated files on npm run clean (#9866)
  chore(pick-list, pick-list-item, value-list-item): fix runtime deprecation messages (#9870)
  refactor: create a common resource to store debounce consts (#9829)
  test(tree): stabilize tests (#9853)
  chore(value-list-item): add runtime deprecation warning (#9863)
  chore: release next
  build(deps): update dependency composed-offset-position to v0.0.6 (#9834)
  feat(dialog): adds new dialog component and deprecates the modal component (#9751)
  chore: release next
  fix(panel, flow-item): prevent footer slots from conflicting with each other (#9856)
calcite-admin pushed a commit that referenced this pull request Jul 30, 2024
**Related Issue:** #5371

## Summary
Refactor: create a common resource to store debounce consts for
`action-bar`, `combobox`, `filter`, `list`, `floating-ui`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants