Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

[FIX] Looping of menu item #2474

Merged
merged 3 commits into from
Dec 6, 2022
Merged

Conversation

stefaandevylder
Copy link
Contributor

@stefaandevylder stefaandevylder commented Dec 5, 2022

Description

Fixes the possibility to have an infinite loop. This was introduced when converting from JS to Typescript. Bug was found with Sentry.

This is a shot in the dark, but most likely the fix.

Sentry: https://sentry.io/organizations/teamleader/issues/3788415931/?project=1211390&referrer=slack

Copy link
Contributor

@lorgan3 lorgan3 left a comment

Choose a reason for hiding this comment

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

Q: Do we need to to anything to make sure to not undo this fix? #2267

@eniskraasniqi1 maybe you remember when the menu opened as an empty square?

qubis741
qubis741 previously approved these changes Dec 5, 2022
KristofColpaert
KristofColpaert previously approved these changes Dec 5, 2022
@eniskraasniqi1
Copy link
Contributor

eniskraasniqi1 commented Dec 5, 2022

Not really 😅, but I'm going to test those changes locally (bring back some lines), and see if I added this line for a good reason.

EDIT: Can't link UI locally, will try with someone tmr.

jelledc
jelledc previously approved these changes Dec 5, 2022
Copy link
Collaborator

@lowiebenoot lowiebenoot left a comment

Choose a reason for hiding this comment

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

Doesn't seem like a good solution to me. The dependency that you now use is a ref, so it will never update, so the effect will never run again.

@stefaandevylder
Copy link
Contributor Author

stefaandevylder commented Dec 6, 2022

@lowiebenoot The item should not be rerendered

Copy link
Contributor

@JorenSaeyTL JorenSaeyTL left a comment

Choose a reason for hiding this comment

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

Shouldnt rerender so 🤞

Copy link
Member

@BeirlaenAaron BeirlaenAaron left a comment

Choose a reason for hiding this comment

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

Sorry didn't see this, could've probably been added in the same release 🙈

@JorenSaeyTL
Copy link
Contributor

Group MCed with @eniskraasniqi1 and @stefaandevylder

@stefaandevylder stefaandevylder merged commit cea22e4 into next-release Dec 6, 2022
@stefaandevylder stefaandevylder deleted the fix/looping-menu-item branch December 6, 2022 10:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants