Skip to content
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

Make ctrl-click open new tab #3723

Merged

Conversation

@theFeu
Copy link
Contributor

commented Mar 20, 2019

refs #3722

Jennifer Mourek

@theFeu theFeu requested a review from lippserd Mar 20, 2019

@nilmerg
Copy link
Member

left a comment

Works very well in the menu. Though, list entries behave weird now. They open in a new tab in addition to the detail view. It also interferes with the selection of multiple rows.

public/js/icinga/events.js Outdated Show resolved Hide resolved
@theFeu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Works very well in the menu. Though, list entries behave weird now. They open in a new tab in addition to the detail view. It also interferes with the selection of multiple rows.

I'll look into this ASAP :)

@theFeu

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@nilmerg
What behaviour would you expect on clicking the list-item?
A new browser tab opening but no detail view?
No tab opening?

This interferes with it being possible to make multiselects with the ctrl/cmd key.

So I see two possibilities on how to handle this: we keep the behaviour as it is (normal multiselect + new tab opeing) or we remove the crtl/cmd keys from triggering multiselect.

@nilmerg

This comment has been minimized.

Copy link
Member

commented May 21, 2019

I generally only expect the ctrl+click behavior to work when directly aiming at an anchor. Doing this on text or empty areas I wouldn't expect this to work. To select multiple elements I rather tend to click anywhere but the anchor.

@theFeu

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

should be fixed now :)

@nilmerg

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Are you sure? Shows the same behavior for me.

@theFeu theFeu force-pushed the feature/ctrl-click-does-not-open-links-in-new-tab-3722 branch from 5e38632 to 26d6963 May 21, 2019

@theFeu

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Fixed me being an idiot too now ^-^'

@nilmerg

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Yep, the host list and services list work fine even with multiselection. Though, some lists still open in a new tab and open a column. (e.g. /icingaweb2/user/list)

@theFeu theFeu requested a review from nilmerg May 22, 2019

@theFeu

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

Yep, the host list and services list work fine even with multiselection. Though, some lists still open in a new tab and open a column. (e.g. /icingaweb2/user/list)

And you think it should not open a new column?
Or no new tab?

Since the multiselect is not possible here, I don't see the problem with it opening a new tab - even when clicking on the row instead of the anchor.
The meta key does not serve any other function in this (except for open new tab, that is).

Plus: I personally really like having a little preview over what i just opened in a new tab :)

@nilmerg

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Well, the default behavior of a browser in this case is to open the target in a new window (for browsers nowadays in a new tab) while the page where the link was clicked stays the same. Consider for a moment someone clicking on a link like this in the right-most column. The left column is lost, what's probably not what the user wanted.

@theFeu

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

The left column is lost, what's probably not what the user wanted.

This is something I definitely didn't think about!

@theFeu theFeu self-assigned this May 23, 2019

public/js/icinga/events.js Outdated Show resolved Hide resolved
@theFeu

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

The Tactical overview would also be less of a special case if we would change the legend to behave like other 'badge collections'.
See: #3797

@nilmerg nilmerg added this to the 2.7.0 milestone May 24, 2019

@nilmerg nilmerg merged commit b4979b7 into master May 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nilmerg nilmerg deleted the feature/ctrl-click-does-not-open-links-in-new-tab-3722 branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.