-
Notifications
You must be signed in to change notification settings - Fork 54.3k
feat(editor): Add base for the setup panel feature (no-changelog) #25010
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Bundle ReportChanges will increase total bundle size by 9.94kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: editor-ui-esmAssets Changed:
Files in
|
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 issues found across 14 files
This comment has been minimized.
This comment has been minimized.
…k' into ADO-4715-setup-panel-initial-work
|
|
||
| @Config | ||
| export class SetupPanelConfig { | ||
| @Env('N8N_SETUP_PANEL_ENABLED') |
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.
Isn't a feature flag more appropriate here instead of env variable? I assume once we're ready with the feature we'd gradually enable this/ab test it which would require ff.
| watch( | ||
| () => resolvedParameter.value, | ||
| (newValue, oldValue) => { | ||
| if (newValue && newValue !== oldValue) { |
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 not sure why is this needed. Should we add a comment explaining?
| <style module lang="scss"> | ||
| .container { | ||
| display: flex; | ||
| padding: var(--spacing--3xs); |
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.
| const props = withDefaults( | ||
| defineProps<{ | ||
| modelValue?: SetupPanelTabs; | ||
| tabLabels?: { |
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.
nit: maybe labelOverrides is a better name for this?
| import { type ContextMenuAction } from '@/features/shared/contextMenu/composables/useContextMenuItems'; | ||
| import { type CanvasNode, CanvasNodeRenderType } from '@/features/workflows/canvas/canvas.types'; | ||
| import { useCanvasOperations } from '@/app/composables/useCanvasOperations'; | ||
| import SetupPanelTabs from '@/features/setupPanel/components/SetupPanelTabs.vue'; |
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 guess with adding the Setup panel it makes more sense to rather have a parent component eg ParentPannel(def not a good name) that is using the tabs and based on the selected tab renders either SetupPanel or Focus panel. It may be an overkill to already do this though especially since for a few more weeks or so for the users there will be only the SetupPanel so the current change is may be more appropriate for the transition so up to you.
| <SetupPanel /> | ||
| </div> | ||
| <div v-else data-test-id="focus-panels" :class="$style.content"> | ||
| <ExperimentalFocusPanelHeader |
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.
This is just the old component with same params moved inside the new container, right? The diff is a little weird


Summary
Related Linear tickets, Github issues, and Community forum posts
Closes ADO-4715
Closes ADO-4716
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)