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.

Sign up for GitHub

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

Jump to bottom

chore(web): typing and refactoring #6948

Merged
merged 7 commits into from
Jul 24, 2024
Merged

chore(web): typing and refactoring #6948

merged 7 commits into from
Jul 24, 2024

Conversation

tonypls
Copy link
Collaborator

@tonypls tonypls commented Jul 3, 2024

Issue

Found some motivation to tidy up some long standing any types

Description

This PR refactors and adds types where I could figure them out. There's still a few more to go but this is a nice start. I've tried to minimize the changes of the actual functional code.

@tonypls tonypls requested a review from VIKTORVAV99 July 3, 2024 14:48
@tonypls
Copy link
Collaborator Author

tonypls commented Jul 3, 2024

/review

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 3, 2024

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Bug:
The refactoring from object to enum in TimeSlider.tsx for COLORS might cause issues if there are other parts of the codebase that still expect COLORS to be an object with nested properties. This could lead to property access errors.

Refactoring Concern:
The change in AreaGraph.tsx introduces new type definitions and refactors functions to be more type-safe. However, this might introduce subtle bugs if not all usages of these functions have been updated to match the new signatures.

@VIKTORVAV99 VIKTORVAV99 mentioned this pull request Jul 3, 2024
Closed
1 task
@VIKTORVAV99 VIKTORVAV99 self-requested a review July 11, 2024 09:36
silkeholmebonnen
silkeholmebonnen approved these changes Jul 11, 2024
View reviewed changes
Copy link
Contributor

@silkeholmebonnen silkeholmebonnen left a comment

Choose a reason for hiding this comment

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

LGTM ⭐ Just one question and an optional suggestion

web/src/features/charts/elements/GraphBackground.tsx Outdated Show resolved Hide resolved
web/src/features/charts/elements/GraphBackground.tsx
Comment on lines -26 to -29
if (mouseOutRectTimeout) {
clearTimeout(mouseOutRectTimeout);
mouseOutRectTimeout = undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that I understand why we had this in the first place. Do you know what the purpose of it was?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No idea but I've learned that if something seems complicated there's probably a good reason why it's in the code, I would only remove this if I could explain why it was there!

VIKTORVAV99
VIKTORVAV99 reviewed Jul 11, 2024
View reviewed changes
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

Just some small comments

web/src/features/charts/elements/GraphBackground.tsx Outdated Show resolved Hide resolved
web/src/translation/i18n.ts Show resolved Hide resolved
@tonypls
Copy link
Collaborator Author

tonypls commented Jul 24, 2024

Add some updates from PR feedback 👍

VIKTORVAV99
VIKTORVAV99 approved these changes Jul 24, 2024
View reviewed changes
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@tonypls tonypls merged commit ab2797c into master Jul 24, 2024
20 checks passed
@tonypls tonypls deleted the tvs/fix-warnings branch July 24, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silkeholmebonnen silkeholmebonnen silkeholmebonnen approved these changes

@VIKTORVAV99 VIKTORVAV99 VIKTORVAV99 approved these changes

Assignees
No one assigned
Labels
frontend 🎨 Review effort [1-5]: 3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tonypls @VIKTORVAV99 @silkeholmebonnen