-
Notifications
You must be signed in to change notification settings - Fork 66
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
Respect CTRL/CMD+Click for opening instance editor in new tab #4800
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4800 +/- ##
=======================================
Coverage 78.72% 78.72%
=======================================
Files 314 314
Lines 15053 15053
Branches 3457 3457
=======================================
Hits 11851 11851
Misses 3202 3202
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@@ -89,7 +89,15 @@ export default { | |||
if (this.disabled) { | |||
return false | |||
} | |||
window.open(this.url, !this.isImmersiveEditor ? '_blank' : '_self') | |||
let href = this.url |
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.
This is a viable solution but for only one specific use case.
My take on this problem would be in the frontend/src/ui-components/components/Button.vue
component, to completely remove the button and used an anchor stylized as a button or switch the type='anchor' where needed on the said component. That way we'd let the default html events take their course and we wouldn't need to intervene for niche cases
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, but the issue is the URL is different depending on accessing the editor direct or immersive. As it stands, we have no option but to have a click handler for the descision stuff and a href for the middle click.
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.
my bad, my solution was focused on other places where this is happening!
@@ -89,7 +89,15 @@ export default { | |||
if (this.disabled) { | |||
return false | |||
} | |||
window.open(this.url, !this.isImmersiveEditor ? '_blank' : '_self') | |||
let href = this.url |
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.
my bad, my solution was focused on other places where this is happening!
Description
Opens the instance editor in a new tab when CTRL/CMD is held
This is not as straight forward as setting target since the URL for immersive is different to the URL for standalone (thought that is something that could potentially be handled with more effort)
For now, this is a simple mod that adds to the current working logic. i.e. currently, we do already support middle mouse button click, this PR builds on that to check the key held and opens in a new tab if it is.
Very much a nice to have but worth the 20 mins
Related Issue(s)
Checklist
flowforge.yml
?FlowFuse/helm
to update ConfigMap TemplateFlowFuse/CloudProject
to update values for Staging/ProductionLabels
area:migration
label