-
Notifications
You must be signed in to change notification settings - Fork 180
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
feature(fe2): View mode FE integration #3651
feature(fe2): View mode FE integration #3651
Conversation
const handler = handlers[shortcut.action as ViewerShortcutAction] | ||
if (handler) { | ||
onKeyboardShortcut([...shortcut.modifiers], shortcut.key, () => { | ||
if (!isTypingComment.value) handler() |
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.
@andrewwallacespeckle I guess this will not block shortcuts when typing elsewhere (feedback modal, filter input etc?)
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.
Good question, will double check and test this. Will leave unresolved until I do
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 used VueUse here and it seems to work well. You can check it out in this commit:
5a68e55
📸 Preview service has generated an image. |
📸 Preview service has generated an image. |
📸 Preview service has generated an image. |
} | ||
}) | ||
} | ||
} |
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.
Yup, this extension was here mostly as a placeholder for the frontend. Removing it is the way to go 👍
@@ -48,5 +63,7 @@ export class ViewModes extends Extension { | |||
break | |||
} | |||
this.viewer.requestRender(UpdateFlags.RENDER_RESET) | |||
|
|||
this.emit(ViewModeEvent.Changed, viewMode) | |||
} | |||
} |
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 like the approach of defining it's own events 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.
All good from my point of view. I'm happy seeing additions to the viewer and it's extensions, especially when they are so on point! Excellent work!
📸 Preview service has generated an image. |
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.
} | ||
} as const | ||
|
||
export const ToolShortcuts = { |
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's not a must, but you can add : readonly
, then TS will complain if you try to modify these consts, I started using that for consts that shouldn't change as well
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.
Code looks good 😄
📸 Preview service has generated an image. |
📸 Preview service has generated an image. |
📸 Preview service has generated an image. |
📸 Preview service has generated an image. |
Description & motivation
Changes:
To-do before merge:
Screenshots:
Validation of changes:
Checklist:
References