Migrate reporting top nav to sharing context menu#22596
Merged
nreese merged 31 commits intoelastic:masterfrom Sep 11, 2018
Merged
Migrate reporting top nav to sharing context menu#22596nreese merged 31 commits intoelastic:masterfrom
nreese merged 31 commits intoelastic:masterfrom
Conversation
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
Author
|
@elastic/kibana-design Where should the feedback go when the user clicks the generate button? Right now a toast notification is displayed. @cjcenizal suggested disabling the button and displaying the message in the context menu. I like that idea since it puts the feedback closer to where the action takes place. What do you think?
|
Contributor
|
I just thought of one problem with my idea of disabling the button... if the user has auto-refresh enabled and wants to create a series of PDFs as the data changes, then disabling it on click will be frustrating. |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
Author
|
bootstrap error |
Contributor
Author
|
jenkins, test this |
Contributor
💔 Build Failed |
Contributor
Author
|
jenkins, test this |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
Author
|
jenkins, test this |
Contributor
💔 Build Failed |
Contributor
💔 Build Failed |
Contributor
Author
|
flaky x-pack security test jenkins, test this |
Contributor
💚 Build Succeeded |
Contributor
💔 Build Failed |
Contributor
Author
|
Same xpack rbac test failure. @elastic/kibana-security Is this a known flaky test? jenkins, test this |
Member
|
@Rasroh is this the flaky test you wanted to debug with me? |
Member
Contributor
💚 Build Succeeded |
stacey-gammon
approved these changes
Sep 11, 2018
stacey-gammon
left a comment
There was a problem hiding this comment.
Couple minor things but otherwise, LGTM. Did not pull down and test latest code.
src/ui/public/share/share_action.ts
Outdated
| readonly id: string; | ||
|
|
||
| getShareActions: ( | ||
| { objectType, objectId, getUnhashableStates, sharingData, isDirty, onClose }: ShareActionProps |
There was a problem hiding this comment.
Interesting way to do this. Since it's just defining the interface and ShareAction, I think you could get away with:
getShareActions: (config: ShareActionProps) => ShareAction[]
Contributor
Author
There was a problem hiding this comment.
not sure if you are looking at an old commits, but the destructuring was removed in this commit
There was a problem hiding this comment.
Sorry, that was an old unpublished comment that got published by accident with the latest :)
| const query = { | ||
| jobParams: rison.encode(jobParams), | ||
| }; | ||
| return kfetch({ method: 'POST', pathname: `${API_BASE_URL}/${exportType}`, query }).then( |
There was a problem hiding this comment.
I think if you make this an async function and use await the returned promise is implicit. Should still work with the catch. E.g.
const response = await kfetch({ method: 'POST', pathname: `${API_BASE_URL}/${exportType}`, query });
jobCompletionNotifications.add(response.job.id);
I don't think you need to return the response, just the promise.
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| // TODO: Remove once typescript definitions are in EUI |
There was a problem hiding this comment.
I think you can remove this comment, there is no declaration here, so I think these defintions are typed in eui
Contributor
💚 Build Succeeded |
Contributor
Author
|
cc @w33ble Not sure how this is going to impact canvas/reports. |
Contributor
💔 Build Failed |
sorenlouv
reviewed
Sep 21, 2018
| "@kbn/ui-framework": "link:../packages/kbn-ui-framework", | ||
| "@samverschueren/stream-to-observable": "^0.3.0", | ||
| "@slack/client": "^4.2.2", | ||
| "@types/moment-timezone": "^0.5.8", |
Contributor
There was a problem hiding this comment.
@nreese Do you mind if I move this to devDependencies?
Contributor
Author
There was a problem hiding this comment.
Not at all. Thanks @sqren. Putting it in dependencies was a mistake
Contributor
There was a problem hiding this comment.
No worries. I just noticed as I was adding some types.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
Dev Docs
exportTypesRegistryremovedRemoved
exportTypesRegistryfrom the client-side.exportTypesRegistryhad been used for registering Reporting export types for the UI.exportTypesRegistrystill exists for registering export types with the server.