-
Notifications
You must be signed in to change notification settings - Fork 321
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
Code editor open-close improvements #9365
Conversation
</script> | ||
|
||
<template> | ||
<div class="ExtendedMenu"> | ||
<div class="moreIcon" @pointerdown="isDropdownOpen = !isDropdownOpen"></div> | ||
<div class="moreIcon" @pointerdown="isDropdownOpen = !isDropdownOpen">…</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.
We would need an icon for that, but for now rotated ellipsis looks good IMO.
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.
Again I think this should already be using an svg 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.
Replaced with SVG
@@ -347,6 +349,7 @@ const editorStyle = computed(() => { | |||
<circle cx="14" cy="14" r="1.5" /> | |||
</svg> | |||
</div> | |||
<div class="closeButton button" @click="emit('close')">+</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.
I really like drawing things with strings, lol. But we would also need an icon here in the future.
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'd rather see an actual placeholder svg icon already being used here (either reuse one of the existing ones, or make a placeholder one), so no styling will need to be adjusted later once an actual icon is prepared.
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.
Replaced with SVG
</script> | ||
|
||
<template> | ||
<div class="ExtendedMenu"> | ||
<div class="moreIcon" @pointerdown="isDropdownOpen = !isDropdownOpen"></div> | ||
<div class="moreIcon" @pointerdown="isDropdownOpen = !isDropdownOpen">…</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.
Since it is not displayed as an ellipsis but rotated by CSS into the intended shape, I think it would be clearer to use CSS to inject the ellipsis character with :before
.
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.
Replaced with SVg
@pointerdown="emit('fitToAllClicked')" | ||
/> | ||
</div> | ||
<div class="row clickableRow" @pointerdown.stop="emit('toggleCodeEditor')"> |
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.
It seems like we should .stop
the pointerdown/pointerup/click events at the root element of the dropdown, and I think attaching these handlers to click
would be more consistent with the way we're handling buttons 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.
Yeah those should be clicks. We should only use pointerdown
in situations where we are dealing with drag interactions, so we need more control than what click
provides.
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.
Replaced with click handlers
@@ -347,6 +349,7 @@ const editorStyle = computed(() => { | |||
<circle cx="14" cy="14" r="1.5" /> | |||
</svg> | |||
</div> | |||
<div class="closeButton button" @click="emit('close')">+</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.
I'd rather see an actual placeholder svg icon already being used here (either reuse one of the existing ones, or make a placeholder one), so no styling will need to be adjusted later once an actual icon is prepared.
@pointerdown="emit('fitToAllClicked')" | ||
/> | ||
</div> | ||
<div class="row clickableRow" @pointerdown.stop="emit('toggleCodeEditor')"> |
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.
Yeah those should be clicks. We should only use pointerdown
in situations where we are dealing with drag interactions, so we need more control than what click
provides.
const emit = defineEmits<{ zoomIn: []; zoomOut: []; fitToAllClicked: [] }>() | ||
const emit = defineEmits<{ zoomIn: []; zoomOut: []; fitToAllClicked: []; toggleCodeEditor: [] }>() | ||
|
||
const toggleCodeEditorShortcut = isMacLike ? 'Cmd + `' : 'Ctrl + `' |
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.
Instead of hardcoding this shortcut string, let's add a new method to the object returned from defineKeybinds
, so that we can read the shortcuts by action name. Then this could be something like codeEditorBindings.displayBinding('toggle')
.
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.
AFAIK, it's not so easy to add the function, as it parses the string and changes the representation.
@vitvakatu If refactoring defineKeybinds would take more than a day, let's make a follow-up issue and add TODO with link here.
When implemented, the function returning keybinds should be also used in area selection unit tests.
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 added the API itself, but I want it to use type information (knowing the list of shortcuts which are actually set). Added a TODO for now, to not block this PR.
</script> | ||
|
||
<template> | ||
<div class="ExtendedMenu"> | ||
<div class="moreIcon" @pointerdown="isDropdownOpen = !isDropdownOpen"></div> | ||
<div class="moreIcon" @pointerdown="isDropdownOpen = !isDropdownOpen">…</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.
Again I think this should already be using an svg icon.
Closes #9411 Based on #9365 <img width="261" alt="image" src="https://github.com/enso-org/enso/assets/6566674/2b419fde-9f66-455e-804b-1690edfb2883">
Pull Request Description
Closes #9209
code-editor-open-close.mp4
Code editor
which toggles visibility of, well, code editorChecklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.