-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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): Inline expression editor #4814
Conversation
ivov
commented
Dec 5, 2022
* ⚡ Extract `ExpressionFunctionIcon` * ⚡ Simplify syntax * ⚡ Move to mixin * 🎨 Format * 📘 Unify types * ⚡ Dedup double brace handler * ⚡ Consolidate resolvable highlighter * 🎨 Format * ⚡ Consolidate language pack * ✏️ Add comment * ⚡ Move completions to plugins * ⚡ Partially deduplicate themes
…4846) * 🎨 Adjust styling for expression parameter input * 🎨 Style outputs differently * ⚡ Set single line for RLC * 🎨 Style both openers identically * 🐛 Prevent defocus on resize * ⚡ Adjust line height
@@ -40,6 +40,7 @@ | |||
"@fortawesome/free-solid-svg-icons": "^5.15.3", | |||
"@fortawesome/vue-fontawesome": "^2.0.2", | |||
"axios": "^0.21.1", | |||
"codemirror-lang-n8n-expression": "^0.1.0", |
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.
can we publish this under n8n? so @n8n_io/codemirror-expression-lang
maybe?
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.
Moving story to P3, should not block release. Btw codemirror-lang-*
is the convention.
> | ||
<ExpressionFunctionIcon /> | ||
</div> | ||
<InlineExpressionEditorInput |
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.
reminder to switch to kebab case
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.
That is what I wanted and argued for and was overruled on because guidelines say the opposite.
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.
ops my bad sorry.. this is good :)
this.$emit('focus'); | ||
}, | ||
getPaneWidth() { | ||
const key = [LOCAL_STORAGE_MAIN_PANEL_RELATIVE_WIDTH, this.currentNodePaneType].join('_'); |
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.
you can see another approach of how to set this here, which might a little better https://github.com/n8n-io/n8n/blob/master/packages/editor-ui/src/components/ResourceLocator/ResourceLocator.vue#L433
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.
The only purpose of tracking pane width was to prevent blur on resize, but seeing as this requires adding to state, watching, comparing to prior width, and possible issues with NDV-to-NDV jumps, I refactored this to rely only on the presence of the resizer
class. This no longer tracks anything and does not fire events on resize, which also (apparently?) even makes dragging to resize snappier than before because we are no longer triggering state changes. Let me know what you think.
|
||
Vue.component('ExpressionFunctionIcon', ExpressionFunctionIcon); | ||
|
||
export default Vue.extend({ |
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.
forgot yesterday.. not must now that you have already implemented this.. but would be great if you can use the composition api for new components..
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.
Ideally best to start simpler and smaller in future. Unfamiliar with composition API so not trivial to refactor all this now.
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.
yes for new components please use composition api
* master: ci: Block all external network calls in tests (no-changelog) (#4930) feat: Update workflow overwriting message (#4917) feat(editor): Update user management setup message when sharing is disabled (#4928) feat(editor): Inline expression editor (#4814) fix: Update permission for showing workflow caller policy (#4916) feat: Update last credential accessor removal message (no-changelog) (#4925) feat: Update credential sharing messages (no-changelog) (#4927) fix: Improve VSCode debugging setup (no-changelog) (#4922)
Got released with |