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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove >0 check so last array item can be removed too #828

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Feb 14, 2024

A small post-mortem:

  • We used to check alertManagerAlerts.length > 0 || alertStackAlerts.length > 0 but the check on alertStackAlerts was removed in chore: fix cli-style useEffect exhaustive deps warnings聽#753.
  • So now the effect won't fire if the alertManagerAlerts is empty but the alertManagerAlerts is still populated. This is a valid use-case which @Birkbjo encountered: if there is only one alert, it cannot be removed programatically
  • This problem did not show up in the test, because we were creating a default alert, which auto-hides. So even though hiding programatically failed this did fail the test, because the alert would eventually auto-hide

I am slightly concerned that removing the condition could cause bugs, but I have no clear indication that it would. I've gone through the commit history of the Alerts.js file and the condition was there since the component started using the hook, so it's hard to say if it has been put there for a reason or if it was there purely to prevent updating state when both arrays are empty. @Birkbjo guessed it would probably be OK, so 馃

@@ -49,9 +49,10 @@ describe('Alerts', () => {

it('can hide an alert', async () => {
const msg = 'TEST ALERT'
const options = { permanent: true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures the alert does't auto-hide. We need this to verify that hiding programmatically does indeed clean up the DOM.

Copy link
Member

@Birkbjo Birkbjo left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants