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 infobox collapse #2237

Merged
merged 2 commits into from
Jul 7, 2024
Merged

Conversation

Kakadus
Copy link
Collaborator

@Kakadus Kakadus commented Jul 1, 2024

fixes a bug, when the info box is closed during opening of the infobox. Now, we hard abort the opening process and further clear the "opening" timeout to prevent the class to be removed unexpectedly.

Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Makes sense, havent tested, thoughts:

evap/static/ts/tsconfig.json Outdated Show resolved Hide resolved
@@ -6,6 +6,7 @@ export class InfoboxLogic {
private readonly infobox: HTMLDivElement;
private readonly closeButton: HTMLButtonElement;
private readonly storageKey: string;
private timeout?: NodeJS.Timeout;
Copy link
Member

Choose a reason for hiding this comment

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

This seems odd, we are not targetting node, so the inferred type should be a browser timeout, not a nodejs one. Do we have typescript misconfigured somewhere? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay: by default, tsc imports all @types/* into the global namespace. As we have @types/node because of jest, this works here.

If I excluded the jest types (and thereby node's types) this compiles with number but compilation of the ts tests fail, see e501844

Copy link
Member

Choose a reason for hiding this comment

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

Ah, okay. According to this, using window.setTimeout (or the returntype trick they use) could work - if not, we can come back to this later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now it works! Adding it to tsconfig.compile.json should work!

evap/static/ts/src/infobox.ts Show resolved Hide resolved
@Kakadus Kakadus requested a review from niklasmohrin July 2, 2024 21:16
@janno42 janno42 self-requested a review July 5, 2024 19:33
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

✅ UI checked

@Kakadus Kakadus force-pushed the fix-infobox-collapse branch from fe81587 to b292222 Compare July 7, 2024 09:20
@Kakadus Kakadus merged commit bb0004e into e-valuation:main Jul 7, 2024
12 checks passed
@Kakadus Kakadus deleted the fix-infobox-collapse branch July 7, 2024 09:26
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.

4 participants