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.

Sign up for GitHub

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

Jump to bottom

[Expressions] Align renderMode with the embeddable viewMode #110199

Merged
merged 6 commits into from
Sep 3, 2021
Merged

[Expressions] Align renderMode with the embeddable viewMode #110199

merged 6 commits into from
Sep 3, 2021

Conversation

dokmic
Copy link
Contributor

@dokmic dokmic commented Aug 26, 2021

Summary

Resolves #108906.

Checklist

For maintainers

@dokmic dokmic added review Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Aug 26, 2021
@dokmic dokmic force-pushed the feature/108906 branch 3 times, most recently from 1dc9165 to 20a5fba Compare August 26, 2021 18:53
@dokmic dokmic marked this pull request as ready for review August 30, 2021 11:31
@dokmic dokmic requested a review from a team August 30, 2021 11:31
@dokmic dokmic requested review from a team as code owners August 30, 2021 11:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Aug 30, 2021
ppisljar
ppisljar requested changes Aug 30, 2021
View reviewed changes
src/plugins/embeddable/common/types.ts Outdated
/**
* Flag indicating that the chart is rendered in a non-interactive environment.
*/
noInteractivity?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

we don't need to add this, disableTriggers is the existing embeddable flag we should use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 👍 I've replaced that.

ppisljar
ppisljar reviewed Aug 31, 2021
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

we need to update visualize_embeddable as well to pass down noInteractivity option to expressions

@timroes timroes requested review from a team and removed request for a team August 31, 2021 15:02
@dokmic
Copy link
Contributor Author

dokmic commented Sep 1, 2021 •
edited
Loading

we need to update visualize_embeddable as well to pass down noInteractivity option to expressions

@ppisljar I've updated that in the latest commit. Could you please take another look?

ppisljar
ppisljar approved these changes Sep 2, 2021
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

stratoula
stratoula approved these changes Sep 2, 2021
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Kibana-vis-editors code LGTM! I tested it locally and works as expected but I would appreciate another look from @dej611 or @mbondyra especially on the Lens side.

dej611
dej611 approved these changes Sep 2, 2021
View reviewed changes
Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

I did some testing locally on Safari and Chrome and it worked ok.
Code looks good.

Probably the only comment I would make is to change the prop noInteractivity to a positive wording interactive, which makes it easier to reason about: it's a nitpick, but I would prefer to use interactive={true/false} rather than noInteractive={false/true}.

poffdeluxe
poffdeluxe approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

presentation team changes lgtm

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
expressions 1597 1598 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
canvas 1.6MB 1.6MB -11.0B
cases 711.4KB 711.4KB -3.0B
lens 1.6MB 1.6MB +201.0B
visualizations 101.5KB 101.5KB +40.0B
total +227.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
embeddable 184.2KB 184.3KB +27.0B
expressions 219.8KB 220.0KB +159.0B
total +186.0B
Unknown metric groups

API count

id before after diff
expressions 2032 2036 +4

History

  • 💚 Build #150902 succeeded 5cc4233b0802a4ac70edc587931a0cff411d4073
  • 💚 Build #150386 succeeded f23d07d7b913858529c81d453658925d44441e81
  • 💔 Build #150305 failed 4f1e4a71a97026db8c171b8048a5b4bb84ed1b84
  • 💚 Build #149584 succeeded 73ea33e66ee9f8878c99486a1368c8dd02e7d2de
  • 💔 Build #149530 failed b77d11e943056854fa13336d0ed6058a4abc5e48

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dokmic dokmic merged commit df43d25 into elastic:master Sep 3, 2021
@dokmic dokmic deleted the feature/108906 branch September 3, 2021 17:10
@dokmic dokmic mentioned this pull request Sep 3, 2021
Merged
dokmic added a commit that referenced this pull request Sep 3, 2021
@dokmic
[Expressions] Align renderMode with the embeddable viewMode (#110199) (…
06ca0c7 
…#111186)

* Add preview view mode to the embeddable
* Rename display render mode to view
* Extract no interactivity render mode to a separate flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dej611 dej611 dej611 approved these changes

@jonathan-buttner jonathan-buttner jonathan-buttner approved these changes

@stratoula stratoula stratoula approved these changes

@ppisljar ppisljar ppisljar approved these changes

@poffdeluxe poffdeluxe poffdeluxe approved these changes

Assignees
No one assigned
Labels
Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes review v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

align expressions renderMode with embeddables viewMode
8 participants
@dokmic @elasticmachine @kibanamachine @poffdeluxe @dej611 @ppisljar @stratoula @jonathan-buttner