-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
add RedirectAppLinks component in kibana_react #67595
Merged
pgayvallet
merged 21 commits into
elastic:master
from
pgayvallet:kbn-58751-redirect-crossapp-links
Jun 12, 2020
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
f18c09f
implements RedirectCrossAppLinks component
pgayvallet d6337a6
update doc
pgayvallet 4c6daa7
Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…
pgayvallet dfd4a2c
review comments
pgayvallet 59efaa6
Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…
pgayvallet 208d367
use `RedirectCrossAppLinks` in SOM SO table page
pgayvallet 9e4a7e1
update snapshots due to merge
pgayvallet e904c9f
Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…
pgayvallet 1797e65
do not filter current app
pgayvallet e0d5b4b
rename component
pgayvallet d88d4d6
fix snapshots
pgayvallet 6def01a
Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…
pgayvallet 61f59f1
add FTR tests
pgayvallet 57fabad
review comments
pgayvallet 7811516
Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…
pgayvallet a8cae8e
Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…
pgayvallet 07cb1f5
remove the `parseAppUrl` unused core API
pgayvallet 6365b2a
fix snapshots
pgayvallet e5820f4
Merge remote-tracking branch 'upstream/master' into kbn-58751-redirec…
pgayvallet 6f0a66a
fix test plugin ts version
pgayvallet 6e90161
add newline
pgayvallet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
222 changes: 222 additions & 0 deletions
222
src/plugins/kibana_react/public/app_links/click_handler.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,222 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import { MouseEvent } from 'react'; | ||
import { ApplicationStart } from 'src/core/public'; | ||
import { createNavigateToUrlClickHandler } from './click_handler'; | ||
|
||
const createLink = ({ | ||
href = '/base-path/app/targetApp', | ||
target = '', | ||
}: { href?: string; target?: string } = {}): HTMLAnchorElement => { | ||
const el = document.createElement('a'); | ||
if (href) { | ||
el.href = href; | ||
} | ||
el.target = target; | ||
return el; | ||
}; | ||
|
||
const createEvent = ({ | ||
target = createLink(), | ||
button = 0, | ||
defaultPrevented = false, | ||
modifierKey = false, | ||
}: { | ||
target?: HTMLElement; | ||
button?: number; | ||
defaultPrevented?: boolean; | ||
modifierKey?: boolean; | ||
}): MouseEvent<HTMLElement> => { | ||
return ({ | ||
target, | ||
button, | ||
defaultPrevented, | ||
ctrlKey: modifierKey, | ||
preventDefault: jest.fn(), | ||
} as unknown) as MouseEvent<HTMLElement>; | ||
}; | ||
|
||
describe('createNavigateToUrlClickHandler', () => { | ||
let container: HTMLElement; | ||
let navigateToUrl: jest.MockedFunction<ApplicationStart['navigateToUrl']>; | ||
|
||
const createHandler = () => | ||
createNavigateToUrlClickHandler({ | ||
container, | ||
navigateToUrl, | ||
}); | ||
|
||
beforeEach(() => { | ||
container = document.createElement('div'); | ||
navigateToUrl = jest.fn(); | ||
}); | ||
|
||
it('calls `navigateToUrl` with the link url', () => { | ||
const handler = createHandler(); | ||
|
||
const event = createEvent({ | ||
target: createLink({ href: '/base-path/app/targetApp' }), | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).toHaveBeenCalledTimes(1); | ||
expect(navigateToUrl).toHaveBeenCalledWith('http://localhost/base-path/app/targetApp'); | ||
}); | ||
|
||
it('is triggered if a non-link target has a parent link', () => { | ||
const handler = createHandler(); | ||
|
||
const link = createLink(); | ||
const target = document.createElement('span'); | ||
link.appendChild(target); | ||
|
||
const event = createEvent({ target }); | ||
handler(event); | ||
|
||
expect(event.preventDefault).toHaveBeenCalledTimes(1); | ||
expect(navigateToUrl).toHaveBeenCalledWith('http://localhost/base-path/app/targetApp'); | ||
}); | ||
|
||
it('is not triggered if a non-link target has no parent link', () => { | ||
const handler = createHandler(); | ||
|
||
const parent = document.createElement('div'); | ||
const target = document.createElement('span'); | ||
parent.appendChild(target); | ||
|
||
const event = createEvent({ target }); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('is not triggered when the link has no href', () => { | ||
const handler = createHandler(); | ||
|
||
const event = createEvent({ | ||
target: createLink({ href: '' }), | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it('is only triggered when the link does not have an external target', () => { | ||
const handler = createHandler(); | ||
|
||
let event = createEvent({ | ||
target: createLink({ target: '_blank' }), | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
|
||
event = createEvent({ | ||
target: createLink({ target: 'some-target' }), | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
|
||
event = createEvent({ | ||
target: createLink({ target: '_self' }), | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).toHaveBeenCalledTimes(1); | ||
expect(navigateToUrl).toHaveBeenCalledTimes(1); | ||
|
||
event = createEvent({ | ||
target: createLink({ target: '' }), | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).toHaveBeenCalledTimes(1); | ||
expect(navigateToUrl).toHaveBeenCalledTimes(2); | ||
}); | ||
|
||
it('is only triggered from left clicks', () => { | ||
const handler = createHandler(); | ||
|
||
let event = createEvent({ | ||
button: 1, | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
|
||
event = createEvent({ | ||
button: 12, | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
|
||
event = createEvent({ | ||
button: 0, | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).toHaveBeenCalledTimes(1); | ||
expect(navigateToUrl).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('is not triggered if the event default is prevented', () => { | ||
const handler = createHandler(); | ||
|
||
let event = createEvent({ | ||
defaultPrevented: true, | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
|
||
event = createEvent({ | ||
defaultPrevented: false, | ||
}); | ||
handler(event); | ||
|
||
expect(event.preventDefault).toHaveBeenCalledTimes(1); | ||
expect(navigateToUrl).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
it('is not triggered if any modifier key is pressed', () => { | ||
const handler = createHandler(); | ||
|
||
let event = createEvent({ modifierKey: true }); | ||
handler(event); | ||
|
||
expect(event.preventDefault).not.toHaveBeenCalled(); | ||
expect(navigateToUrl).not.toHaveBeenCalled(); | ||
|
||
event = createEvent({ modifierKey: false }); | ||
handler(event); | ||
|
||
expect(event.preventDefault).toHaveBeenCalledTimes(1); | ||
expect(navigateToUrl).toHaveBeenCalledTimes(1); | ||
}); | ||
}); |
56 changes: 56 additions & 0 deletions
56
src/plugins/kibana_react/public/app_links/click_handler.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import React from 'react'; | ||
import { ApplicationStart } from 'src/core/public'; | ||
import { getClosestLink, hasActiveModifierKey } from './utils'; | ||
|
||
interface CreateCrossAppClickHandlerOptions { | ||
container: HTMLElement; | ||
navigateToUrl: ApplicationStart['navigateToUrl']; | ||
} | ||
|
||
export const createNavigateToUrlClickHandler = ({ | ||
container, | ||
navigateToUrl, | ||
}: CreateCrossAppClickHandlerOptions): React.MouseEventHandler<HTMLElement> => { | ||
return (e) => { | ||
if (container == null) { | ||
return; | ||
} | ||
// see https://github.com/DefinitelyTyped/DefinitelyTyped/pull/12239 | ||
const target = e.target as HTMLElement; | ||
|
||
const link = getClosestLink(target, container); | ||
if (!link) { | ||
return; | ||
} | ||
|
||
if ( | ||
link.href && // ignore links with empty hrefs | ||
(link.target === '' || link.target === '_self') && // ignore links having a target | ||
e.button === 0 && // ignore everything but left clicks | ||
!e.defaultPrevented && // ignore default prevented events | ||
!hasActiveModifierKey(e) // ignore clicks with modifier keys | ||
) { | ||
e.preventDefault(); | ||
navigateToUrl(link.href); | ||
} | ||
}; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch B.V. licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
export { RedirectAppLinks } from './redirect_app_link'; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could use built-in Element.closest() method?
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.
Not sure, due to browser support: https://caniuse.com/#search=closest?
Unfortunately I don't have a browserstack account to ensure this API is properly shimmed by
corejs
Do we have a usable account somewhere?
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 are discounting IE11 support in v7.9, so it's not a problem.
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.
That requires quite a bit of changes in the handler unit tests as it can no longer pass down the container for testing, but I guess using native APIs should be encouraged.
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.
Hum. it seems that our version of
jsdom
doesnt supportsclosest
, which is strange as it's supposed to be available for quite a long time 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.
Confirmed: the
jsdom
version that (our version of) jest is using (node_modules/jest-environment-jsdom/node_modules/jsdom/lib/jsdom/living/nodes/Element-impl.js
) is a11.5.1
, which does not implementsclosest
...Latest jest does have a version that implements it though (https://github.com/facebook/jest/blob/master/packages/jest-environment-jsdom/package.json)
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.
That's a shame. Let's keep the current implementation then. We can add a comment to switch to native API later.
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.
Will add a comment for when #58095 lands.
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.
Unfortunately, I wasn't able to bring the most recent version of
jsdom
in that PR, because seems a combination ofjest@25
withjsdom>=14
got serious performance degradation and we cannot bump tojest@26
, because it requirestypescript>=3.8
, so I hope @restrry PR gets merged soon and I will give it a try tojest@26
andjsdom@16