-
-
Notifications
You must be signed in to change notification settings - Fork 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: keyboard shortcut added for opening editor modal #2213
feat: keyboard shortcut added for opening editor modal #2213
Conversation
Your free trial has expired. To keep using Ellipsis, sign up at https://app.ellipsis.dev for $20/seat/month or reach us at help@ellipsis.dev |
@azmy60 wanna review this? If good, approve and merge! |
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.
Just a some notes, and we're good :)
Also I think there is some merge conflicts too, can you also look into it? Thanks!
const range: RangeComponent = _.last(this.tabulator.getRanges()) | ||
const cell = range.getCells().flat()[0]; | ||
if (this.isEditorMenuDisabled(cell)) return | ||
this.$refs.editorModal.openModal(cell.getValue(), undefined, cell) |
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 you be able to add a FIXME note here so we can avoid calling child methods directly like this? Normally, it should be done by calling an event using this.$modal.show(modalName)
or this.$trigger(AppEvent.something)
.
@@ -411,6 +411,7 @@ export default Vue.extend({ | |||
// const focusingTable = this.tabulator.element.contains(document.activeElement) | |||
// if (!focusingTable) this.page-- | |||
// } | |||
result['alt+m'] = this.openEditorMenuByShortcut.bind(this) |
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.
Should we press shift+enter
(DBeaver uses this)? Or anything similar with pressing enter
? I usually look at other tools to decide. I'm just not too familiar with alt+m
though I feel like it might works or I might be too nitpicky. Feel free to use anything that's best for you.
hi @azmy60 ! thanks for the review! |
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.
Perfect! Thanks so much.
resolves: #1923
shift + enter keyboard shortcut added for opening editor modal