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

enhanced ctrl + click new tab opening feature #1248

Merged
merged 3 commits into from
Jul 20, 2020
Merged

enhanced ctrl + click new tab opening feature #1248

merged 3 commits into from
Jul 20, 2020

Conversation

samcaspus
Copy link
Contributor

@samcaspus samcaspus commented Jul 20, 2020

Changes

modified collapseSidebar function and added the url parameter so if any sidebar is being clicked the user would be ab
open a new tab upon have ctrl button pressed.
Not sure if any specific contributing guidelines are there do let me know incase I miss out anything.

Reference to issue #1187 , #850
code review: @timgl @mariusandra

checklist

  • All querysets/queries filter by Team (if applicable)
  • Backend tests (if applicable)
  • Cypress E2E tests (if applicable)

@timgl
Copy link
Collaborator

timgl commented Jul 20, 2020

Hey, thanks for creating this PR! I think the actual issue is with the Link.js component, not the sidebar. I suggest adding something to the onClick listener in that component that just returns if ctrl (or command in the case of mac) is held down

@samcaspus
Copy link
Contributor Author

samcaspus commented Jul 20, 2020

Hey @timgl my bad didn't realize it could have been that minor change.
made the changes as you said.
Thanks

Copy link
Collaborator

@timgl timgl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

@@ -7,6 +7,10 @@ export function Link({ to, preventClick = false, ...props }) {
href={to || '#'}
{...props}
onClick={(event) => {
if (event.ctrlKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do if (event.metaKey || event.ctrlKey) so that it works on mac as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure will add it right away

@@ -7,6 +7,10 @@ export function Link({ to, preventClick = false, ...props }) {
href={to || '#'}
{...props}
onClick={(event) => {
if (event.ctrlKey) {
window.open(to, '_blank')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do return window.open(to, '_blank')? Now if you open it in a new tab it'll also load the page in the current tab

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 😄

@samcaspus samcaspus requested a review from timgl July 20, 2020 10:20
@timgl
Copy link
Collaborator

timgl commented Jul 20, 2020

Awesome, thanks!

@timgl timgl merged commit 25d9f87 into PostHog:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants