-
Notifications
You must be signed in to change notification settings - Fork 103
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
Upgrade to CodeMirror 6 #2058
base: master
Are you sure you want to change the base?
Upgrade to CodeMirror 6 #2058
Conversation
- upgrade to CM6 - migrated editor line highlighter composable TODO - code completion - test migration
Removed obsolete CSS
@grolu You need rebase this pull request with latest master branch. Please check. |
3d3f503
to
aa0192d
Compare
- Restructured main scss theme dependant classes
Make editor always consume 100% height Cleaned-up some comments
…m:gardener/dashboard into enh/upgrade_cm_6
let [startLine, endLine = startLine] = values | ||
const [startLineCurrent, endLineCurrent] = valuesCurrent | ||
|
||
if (endLine === startLine && startLine === startLineCurrent && !endLineCurrent) { |
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 had similar code in my PR where I introduce the feature to clear the selected line if the current selected line is clicked again, however holger propsed to use ESC-key instead to clear the selection. Clearing with ESC-key currently does not work anymore
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 know. The problem here is that I did not find a way to add a key listener to the gutter. It is possible to add a key listener to the editor itself and clear the highlight on escape. However, the editor needs to be focused, so you would need to click inside the editor to make this work.
So I checked how Github does the highlighting and it does not clear the selection on escape, it works exactly like I implemented it now.
# Conflicts: # .pnp.cjs # .yarn/cache/codemirror-npm-5.65.18-debbbac10b-806e00c708.zip # frontend/src/composables/useShootEditor/helper.js # frontend/src/composables/useShootEditor/index.js # yarn.lock
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.
minor things, otherwise /lgtm
<div v-bind="slotProps.props"> | ||
<g-action-button | ||
size="x-small" | ||
:icon="renderWhitespaces ? 'mdi-grid' : 'mdi-grid-off'" |
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 icons are not intuitive to me. I would use these ones. We should use Hide and Show (not Render). And I think we can delete the tab icon in assets?
<v-tooltip location="top">
<template #activator="slotProps">
<div v-bind="slotProps.props">
<v-btn
variant="text"
size="x-small"
flat
icon
color="black"
@click="renderWhitespaces = !renderWhitespaces"
>
<span v-html="renderWhitespaces ? whitespaceEye : whitespaceEyeOff" style="width: 18px; height: 18px"/>
</v-btn>
</div>
</template>
<span>{{ renderWhitespaces ? 'Hide' : 'Render' }} whitespaces</span>
</v-tooltip>
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.
@petersutter WDYT?
Also, do you think we should render line breaks at all?
Moreover, the setting is not stored anywhere currently. Should we store the state in local storage?
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 suggestion sounds good to me.
Also, do you think we should render line breaks at all?
I think we should not render the line breaks, it does not give much value
Moreover, the setting is not stored anywhere currently. Should we store the state in local storage?
hm, yes probably this should be stored in local storage
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.
Okay
- removed the new line symbols.
- included the proposed icons
- store whitespace setting in local storage
# Conflicts: # .pnp.cjs
- removed the new line symbols. - included the proposed icons - store whitespace setting in local storage
@@ -246,6 +255,27 @@ const showToolbar = computed(() => { | |||
return !isReadOnly.value && !props.hideToolbar | |||
}) | |||
|
|||
const whitespaceEye = computed(() => { | |||
return ` | |||
<svg viewBox="0 0 24 24" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> |
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.
Import the svg images from the assets folder.
import whitespaceEye from '@/assets/whitespace-eye.svg?raw'
import whitespaceEyeOff from '@/assets/whitespace-eye-off.svg?raw'
What this PR does / why we need it:
Upgrade CodeMirror 5 to CodeMirror 6
Which issue(s) this PR fixes:
Fixes #1892
Special notes for your reviewer:
Release note: