-
Notifications
You must be signed in to change notification settings - Fork 334
V3 UI - Tabs & Menu rework #4374
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
Conversation
- move tabs to top - improve tab resizing/scrolling - rework menu - reogranize top/side buttons
- Move to command bar - Add dynamic overflow collapsing
- breadcrumb collapse tweaks - hot reload element observer fixes
- fix some tests
…breaking in future
- increase size of logo/menu area with normal sidebar, add right spacing - fix chevron icon inconsistency
- add renaming by double clicking breadcrumb - tweak submenu positions - show menu items on hover (hack)
674f0ef
to
712cc89
Compare
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.
These are all changes which don't have to block merging, but we could consider adjusting in follow-up PRs:
High Priority
-
[src/components/breadcrumb/SubgraphBreadcrumb.vue] - Issue: Brittle overflow calculation with hardcoded values. Suggestion: Explore CSS-based solutions or dynamic constant derivation.
-
[src/components/breadcrumb/SubgraphBreadcrumbItem.vue] - Issue: Direct DOM manipulation for width. Suggestion: Use reactive Vue styling.
-
[src/components/topbar/CommandMenubar.vue:233-239] - Issue: Direct manipulation of undocumented PrimeVue internal property (
.dirty
). Suggestion: Avoid internal hacks; find public API or implement custom logic. HIGH PRIORITY FIX. -
[src/components/topbar/CommandMenubar.vue] - Issue: Use of
!important
in CSS. Suggestion: Prefer more specific selectors or component props. -
[src/components/topbar/WorkflowTabs.vue] - Issue: Inconsistent horizontal scroll amount calculation. Suggestion: Refine
scrollAmount
for consistent behavior across devices. -
[src/components/breadcrumb/SubgraphBreadcrumbItem.vue:35] - Issue: Extreme z-index (10000). Suggestion: Use design system value (1100-1200).
Medium Priority
-
[src/components/breadcrumb/SubgraphBreadcrumb.vue:58-64] - Issue: Unsafe type casting for DOM access. Suggestion: Use proper TypeScript typing with null checks.
-
[src/composables/element/useOverflowObserver.ts:34-37] - Issue: Only checks horizontal overflow. Suggestion: Add option for vertical overflow detection.
-
[src/components/topbar/WorkflowTabs.vue:220] - Issue: Hardcoded scroll amount (20px). Suggestion: Make configurable or viewport-based.
-
[src/components/topbar/WorkflowTabs.vue:376-386] - Issue: Complex CSS with magic numbers and experimental env(). Suggestion: Add fallback styles and document compatibility.
-
[src/components/topbar/CommandMenubar.vue:114-125] - Issue: Recursive function without depth limit. Suggestion: Add maximum recursion depth protection.
Low Priority
-
[src/utils/mouseDownUtil.ts:3-7] - Issue: No input validation. Suggestion: Add type guards and parameter validation.
-
[src/components/breadcrumb/SubgraphBreadcrumbItem.vue:162-167] - Issue: Relying on event.detail for click detection. Suggestion: Use more robust double-click handling.
-
[src/components/breadcrumb/SubgraphBreadcrumb.vue:133-151] - Issue: Complex manual width calculation. Suggestion: Use ResizeObserver or getBoundingClientRect().
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
┆Issue is synchronized with this Notion page by Unito