Merged
Conversation
Member
|
I tried to test this and got this error. |
cqliu1
force-pushed
the
feat/lens-embeds-in-canvas
branch
6 times, most recently
from
cfc9227 to
ab5a817
Compare
Contributor
Author
|
@timductive I never ran into the issue you encountered, but the branch has been updated and should be working now. |
cqliu1
force-pushed
the
feat/lens-embeds-in-canvas
branch
from
8d7f860 to
5e37003
Compare
cqliu1
added
enhancement
Contributor
|
Pinging @elastic/kibana-canvas (Team:Canvas) |
cqliu1
added
loe:x-large
ryankeairns
approved these changes
Mar 9, 2020
Contributor
ryankeairns
left a comment
ryankeairns
left a comment
There was a problem hiding this comment.
LGTM from the design and functionality side, works as expected/described.
This is going to be a huge win for Canvas and Lens!
Thanks for moving those styles to a better home :)
Member
|
@cqliu1 I spoke with @rayafratkina and let's throw the "b" beta icon next to the elements in the flyout to get this in 7.7 |
Contributor
Author
I'm not sure it's possible to add a beta icon next to each Lens object in our embeddable flyout. We're using the |
cqliu1
force-pushed
the
feat/lens-embeds-in-canvas
branch
from
52c049a to
d487046
Compare
crob611
suggested changes
Mar 16, 2020
Contributor
crob611
left a comment
crob611
left a comment
There was a problem hiding this comment.
Looks good. Let's just clean up for sure the way we determine if it's edit vs view mode.
x-pack/legacy/plugins/canvas/canvas_plugin_src/renderers/embeddable/embeddable.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/canvas/canvas_plugin_src/renderers/embeddable/embeddable.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,33 @@ | |||
| .canvasEmbeddable { | |||
| .embPanel { | |||
| border: none; | |||
Contributor
There was a problem hiding this comment.
Maybe more of a @ryankeairns concern, but I think we should keep the border at least in edit mode. It feels weird having the gear hanging out in the top disconnected from everything, but the border at least boxes it in and gives you some clue that all of that is tied in together.
Fixed import embedded panel styles (#58654) Merging to WIP draft branch
Contributor
Author
|
Update: I removed the changes for dynamically setting the embeddable view mode based on app state. The way I had it working by retrieving the entire app state from redux every time an embeddable was rendered, and that's not ideal. I'll tackle the view mode enhancement in a separate PR. |
cqliu1
force-pushed
the
feat/lens-embeds-in-canvas
branch
from
485f8de to
bc74d9e
Compare
cqliu1
force-pushed
the
feat/lens-embeds-in-canvas
branch
from
bc74d9e to
d6d43ed
Compare
crob611
approved these changes
Mar 19, 2020
cqliu1
added a commit
that referenced
this pull request
Mar 20, 2020
* Added lens embeddables to embed flyout Fixed import embedded panel styles (#58654) Merging to WIP draft branch * Added i18n strings for savedLens * Added tests for lens embeddables * Updated tests * Updated tests * Added style overrides for lens table * DDisables triggers on lens emebeddable * Updated test * Sets embeddable view mode according to app state * Fix embeddable component * Removed embeddable view mode logic * Removed unused import
Contributor
💔 Build FailedTest FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/discover/_discover_histogram·js.discover app discover histogram should visualize monthly data with different day intervalsStandard OutStack TraceKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/visualize/_area_chart·js.visualize app area charts axis scaling does not scale top hit aggStandard OutStack TraceKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/visualize/_area_chart·js.visualize app area charts axis scaling does not scale top hit aggStandard OutStack Traceand 1 more failures, only showing the first 3. History
To update your PR or re-run it, just comment with: |
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
6 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.
Summary
This adds support for Lens embeddables within Canvas and allows Lens visualizations to be added to workpads.
Other Notable Changes
Default styles for embeddable wrapper
We applied CSS overrides to reset background color and border on the wrapper component for all embeddables
Before
After
Note: There is a little funkiness with the Lens charts that allow you to toggle the visibility of each series by clicking on the labels in the legend. It doesn't persist if you turn off a series or two and switch to fullscreen mode or refresh the the page. Users can get around this by filtering out the series they don't want to see, so it's a weird UX edge case that we don't necessarily need to fix for. Ideally we would disable these interactions in Canvas, but for now, I don't think it's a blocker for this feature.
Checklist
Delete any items that are not applicable to this PR.
For maintainers