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

ref(issue-views): Fix query count animation #83624

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

MichaelSun48
Copy link
Member

@MichaelSun48 MichaelSun48 commented Jan 16, 2025

Reverts the latest commit in this PR by adding back the use of and effect and state to achieve the desired animation behavior for issue counts.

Implements the query count animation in a way that achieves the desired behavior without using state and effect

Relevant discussion

This is currently in prod:

Screen.Recording.2025-01-16.at.2.56.13.PM.mov

When you change the search query (not the page filters), the bubble flashes to 0 and shrinks before resizing once the response comes in.

@MichaelSun48 MichaelSun48 requested a review from a team as a code owner January 16, 2025 23:12
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 16, 2025
@MichaelSun48 MichaelSun48 enabled auto-merge (squash) January 16, 2025 23:14
@MichaelSun48 MichaelSun48 disabled auto-merge January 17, 2025 17:15
@MichaelSun48 MichaelSun48 changed the title ref(issue-views): Use effect and state for query counts again ref(issue-views): Fix query count animation Jan 17, 2025
} else if (isError) {
setCount(0);
}
}, [queryCount, isFetching, isError, view.query, view.unsavedChanges]);
Copy link
Member

Choose a reason for hiding this comment

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

you might just want a useMemoWithPrevious here

Copy link
Member Author

Choose a reason for hiding this comment

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

About to push a commit that does the same thing but with no hooks (and just some hackiness), detailed here

Copy link

codecov bot commented Jan 17, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
8590 1 8589 2
View the top 1 failed tests by shortest run time
IssueViewsHeader Issue views query counts should render the correct count for multiple views
Stack Traces | 1.02s run time
Error: Found multiple elements with the text: 42

Here are the matching elements:

Ignored nodes: comments, script, style
...

Ignored nodes: comments, script, style
...

Ignored nodes: comments, script, style
...

(If this is intentional, then use the `*AllBy*` variant of the query (like `queryAllByText`, `getAllByText`, or `findAllByText`)).

Ignored nodes: comments, script, style
...
    at waitForWrapper (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:163:27)
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:86:33
    at Object.<anonymous> (.../views/issueList/issueViewsHeader.spec.tsx:803:48)
    at Promise.then.completed (.../sentry/sentry/node_modules/jest-circus/build/utils.js:298:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (.../sentry/sentry/node_modules/jest-circus/build/utils.js:231:10)
    at _callCircusTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:316:40)
    at _runTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:252:3)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:126:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
    at run (.../sentry/sentry/node_modules/jest-circus/build/run.js:71:3)
    at runAndTransformResultsToJestFormat (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
    at jestAdapter (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
    at runTestInternal (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:367:16)
    at runTest (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:444:34)
    at Object.worker (.../sentry/sentry/node_modules/jest-runner/build/testWorker.js:106:12)

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@MichaelSun48 MichaelSun48 merged commit fa0cf00 into master Jan 17, 2025
42 checks passed
@MichaelSun48 MichaelSun48 deleted the msun/issueViews/useStateAndEffectForCountsAgain branch January 17, 2025 21:14
andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025
~Reverts the latest commit in [this
PR](#83515) by adding back the
use of and effect and state to achieve the desired animation behavior
for issue counts.~

Implements the query count animation in a way that achieves the desired
behavior without using state and effect

[Relevant
discussion](#82990 (comment))

This is currently in prod: 


https://github.com/user-attachments/assets/6c64d85c-6e39-41a5-b11e-6ceeaafa76e7

When you change the search query (not the page filters), the bubble
flashes to 0 and shrinks before resizing once the response comes in.
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.

3 participants