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: regression in delay notification state behavior #1241

Merged
merged 6 commits into from
Jun 16, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented Jun 14, 2024

fix regression issue from #1232 when using delayNotificationState setting.

Before

Screen.Recording.2024-06-15.at.1.50.03.PM.mov

After

Screen.Recording.2024-06-15.at.1.52.53.PM.mov

@setchy setchy added the bug Something isn't working label Jun 14, 2024
@setchy setchy added this to the Release 5.9.0 milestone Jun 14, 2024
@afonsojramos
Copy link
Member

afonsojramos commented Jun 14, 2024

What is the regression exactly? Does this not remove the notification before the animation ends?

I opted for this approach because of that. Maybe I should have asked that on the pr 😅

@setchy
Copy link
Member Author

setchy commented Jun 14, 2024

The regression is the row doesn't get set as opaque and left until the next refresh interval.

Instead, it's removed but left with a blank gap/void. Particularly noticeable when you interact with the last notification for a repository as it leaves the repository heading with a void below it

UPDATE: added recording to the OP

@setchy
Copy link
Member Author

setchy commented Jun 14, 2024

Happy to hear if there is a better way 😎

src/utils/remove-notification.ts Outdated Show resolved Hide resolved
@setchy setchy requested a review from afonsojramos June 16, 2024 10:43
@afonsojramos afonsojramos merged commit caff503 into main Jun 16, 2024
8 checks passed
@afonsojramos afonsojramos deleted the fix/delay-notification-state branch June 16, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants