-
-
Notifications
You must be signed in to change notification settings - Fork 656
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
Feat: sticky insights header and widget tooltip icon #6537
Conversation
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 1 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
🚩 Declining Code Health (highest to lowest):
- Complex Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
✅ Improving Code Health:
- Large Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
export const ExecutiveDashboard: VFC = () => { | ||
const [scrolled, setScrolled] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Complex Method
ExecutiveDashboard:VFC has a cyclomatic complexity of 10, threshold = 10
Why does this problem occur?
This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring. Read more.
To ignore this warning click here.
@@ -85,7 +109,7 @@ export const ExecutiveDashboard: VFC = () => { | |||
/> | |||
} | |||
/> | |||
</Box> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No longer an issue: Large Method
ExecutiveDashboard:VFC is no longer above the threshold for lines of code
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 1 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
🚩 Declining Code Health (highest to lowest):
- Complex Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
✅ Improving Code Health:
- Large Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'm curious about the complexity needed to record the scrolling
position: 'sticky', | ||
top: 0, | ||
zIndex: 1000, | ||
padding: scrolled ? theme.spacing(2, 0) : theme.spacing(0, 0, 2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what this does in practice? Why does the padding need to be different for the header once we scroll?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this gets pinned to to the top it looks weird with 0 padding (the search input label is almost half visible). When you are at the top - even the 2 px padding makes it inconsistent with other pages with empty breadcrumbs. This with the small transition makes it look ok. I added a video of how it looks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 1 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
🚩 Declining Code Health (highest to lowest):
- Complex Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
✅ Improving Code Health:
- Large Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
Signed-off-by: andreas-unleash <andreas@getunleash.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 2 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 0 files(s) 🔥
Recommended Review Level: Lightweight sanity check
View detailed results in CodeScene
🚩 Declining Code Health (highest to lowest):
- Complex Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
- Large Method createChartOptions.ts: createOptions
✅ Improving Code Health:
- Large Method ExecutiveDashboard.tsx: ExecutiveDashboard:VFC
@@ -27,13 +27,15 @@ export const createOptions = ( | |||
}, | |||
tooltip: { | |||
enabled: false, | |||
position: 'nearest', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Getting worse: Large Method
createOptions increases from 73 to 75 lines of code, threshold = 70
Why does this problem occur?
Large functions with many lines of code are generally harder to understand and lower the code health. Avoid adding more lines to this function. Read more.
To ignore this warning click here.
What it says on the tin
Closes # 1-2182
Screen.Recording.2024-03-14.at.11.27.52.mov