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

fix(query-builder): Improve the stability of token keys #70828

Merged
merged 1 commit into from
May 14, 2024

Conversation

malwilley
Copy link
Member

Previously, the cursor would jump around when entering a free text portion due to the component remounting because the key changed. This makes some changes to avoid keys changing as much as possible:

  • Updates token keys from {type}:{cursor_location} to {type}:{type_index}
  • Renames space tokens to free_text since they are treated the same and it keeps the keys stable

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 13, 2024
@malwilley malwilley force-pushed the malwilley/fix/search-text-token-type branch from 59f27d8 to bdf9b20 Compare May 14, 2024 18:36
@malwilley malwilley force-pushed the malwilley/fix/search-text-token-type branch from bdf9b20 to d4b21e8 Compare May 14, 2024 18:36
@malwilley malwilley requested a review from a team May 14, 2024 18:37
@malwilley malwilley marked this pull request as ready for review May 14, 2024 18:37
// XXX(malwilley): SearchQueryBuilderInput updates state in the render
// function which causes an act warning despite using userEvent.click.
// Cannot find a way to avoid this warning.
jest.spyOn(console, 'error').mockImplementation(jest.fn());
Copy link
Member Author

Choose a reason for hiding this comment

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

Updating the state in a useEffect makes this warning go away, but does cause other issues with having a stale value for a render cycle so I don't want to do that. I tried wrapping everything I could in act to no avail 😞

Hoping I can eventually get to the bottom of this, but throwing in the towel for now

Copy link

codecov bot commented May 14, 2024

Bundle Report

Changes will decrease total bundle size by 8 bytes ⬇️

Bundle name Size Change
app-webpack-bundle-array-push 26.8MB 8 bytes ⬇️

Copy link
Member

@scttcper scttcper left a comment

Choose a reason for hiding this comment

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

that act warning will haunt us forever

@malwilley malwilley merged commit 8dd7270 into master May 14, 2024
41 checks passed
@malwilley malwilley deleted the malwilley/fix/search-text-token-type branch May 14, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants