-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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(editor): Add override parameterInput field functionality (no-changelog) #12587
base: master
Are you sure you want to change the base?
Conversation
function parseOverrides(s: string) { | ||
/* | ||
Approaches: | ||
- Smart and boring: Reuse packages/core/src/CreateNodeAsTool.ts - unclear where to move it |
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.
Maybe moving it to the workflow package, which a dependency of the the editor-ui package and then you can also use it here?
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.
workflow package is also used by the backend. I would recommend we only add it there if it's also needed by the backend.
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.
editor-ui, core, and CLI use the workflow package. If only two of those packages use the functionality we can move it, as it's better than duplicating code.
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.
So far, it's not shared AFAIK. Unless I missing something.
fill-rule="evenodd" | ||
clip-rule="evenodd" | ||
d="M15.7982 7.80784L13.92 7.20243C12.7186 6.80602 12.0148 5.83386 11.6844 4.61226L10.8579 0.586096C10.8363 0.506544 10.7837 0.400024 10.6219 0.400024C10.4857 0.400024 10.4075 0.506544 10.386 0.586096L9.55943 4.61361C9.22773 5.83521 8.52525 6.80737 7.32387 7.20378L5.44562 7.80919C5.18 7.89548 5.17595 8.27032 5.44023 8.36066L7.33196 9.01191C8.52929 9.40968 9.22773 10.3805 9.55943 11.5967L10.3873 15.5784C10.4089 15.6579 10.4534 15.8008 10.6233 15.8008C10.7991 15.8008 10.8362 15.6634 10.858 15.5831L10.8592 15.5784L11.6871 11.5967C12.0188 10.3791 12.7173 9.40833 13.9146 9.01191L15.8063 8.36066C16.0679 8.26897 16.0639 7.89413 15.7982 7.80784ZM5.04114 11.3108C3.92815 10.9434 3.81743 10.5296 3.63184 9.83597L3.62672 9.81687L3.15615 8.16649C3.12784 8.05997 2.85008 8.05997 2.82041 8.16649L2.50085 9.69147C2.31074 10.394 1.90623 10.9522 1.21588 11.18L0.11563 11.6574C-0.0367335 11.7072 -0.0394302 11.923 0.112933 11.9742L1.22127 12.3666C1.90893 12.5945 2.31074 13.1527 2.5022 13.8525L2.82176 15.3114C2.85142 15.4179 3.12784 15.4179 3.15615 15.3114L3.53099 13.8592C3.72111 13.1554 4.01235 12.5958 4.94675 12.3666L5.98768 11.9742C6.14004 11.9216 6.13869 11.7059 5.98498 11.656L5.04114 11.3108ZM5.33019 0.812949C5.36674 0.661849 5.58158 0.659434 5.61894 0.811355L5.61899 0.811582L6.02856 2.50239C6.08442 2.69624 6.23624 2.8465 6.43132 2.89951L7.47286 3.18013C7.61383 3.2197 7.61829 3.41714 7.48035 3.46394L7.48015 3.46401L6.38799 3.83076C6.21241 3.88968 6.07619 4.03027 6.02153 4.20719L5.61894 5.77311L5.61884 5.77349C5.58095 5.92613 5.36829 5.91987 5.33166 5.77336L4.94237 4.21215C4.88888 4.03513 4.75378 3.89336 4.57956 3.83328L3.48805 3.4555C3.34919 3.40591 3.36033 3.20859 3.50031 3.17175L3.50054 3.17169L4.53472 2.90337C4.73486 2.85153 4.89134 2.69755 4.94463 2.49805L5.33019 0.812949Z" | ||
fill="#C3C9D5" |
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.
Is there a light mode version for this icon?
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.
light mode exists 🤦 ty
packages/design-system/src/components/N8nInputLabel/InputLabel.vue
Outdated
Show resolved
Hide resolved
@@ -1505,7 +1551,14 @@ onUpdated(async () => { | |||
<InlineExpressionTip /> | |||
</div> | |||
</div> | |||
|
|||
<div |
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.
Would be nice to only render this wrapper div if there's a slot
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.
For some reason the slot exists even if the v-if condition for its template provider is not met:
<template v-if="showOverrideButton && isSingleLineInput" #overrideButton>
even with this $slots.overrideButton
is truthy :/
}; | ||
} | ||
const isContentOverride = computed(() => isOverrideValue(props.value?.toString() ?? '')); |
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.
nitpick - typo isContentOverriden
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.
hmm, this is meant as "is (a) Content Override.", not "is (this) content overridden?". Definitely want a better name here
@@ -90,6 +91,7 @@ type Props = { | |||
hideIssues?: boolean; | |||
errorHighlight?: boolean; | |||
isForCredential?: boolean; | |||
canBeOverride?: boolean; |
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.
Do we need this prop? Can we simply check if the slot is set or not?
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.
Not for the edge case where we have the button in the options above, but need a 90deg top right corner for the (multi-line) expr editor
@@ -199,10 +277,16 @@ watch( | |||
} | |||
}, | |||
); | |||
const parameterInputWrapper = useTemplateRef('parameterInputWrapper'); | |||
const isSingleLineInput: ComputedRef<boolean> = computed( |
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.
Why do we type it as ComputedRef<boolean>
and not make the assignation directly.
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 technically recursively somewhere further down, which is why it needs an explicit declaration
Summary
This branch is for review only, once approved I will create a feature branch and merge this as its first (squashed) commit.
Noted omissions/ToDos:
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/ADO-3034/feature-add-fieldoverride-functionality-to-parameterinputfull
Review / Merge checklist
release/backport
(if the PR is an urgent fix that needs to be backported)