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

Improve vis editor typings #80004

Merged
merged 5 commits into from
Oct 13, 2020
Merged

Improve vis editor typings #80004

merged 5 commits into from
Oct 13, 2020

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Oct 8, 2020

Summary

This improves the typings of the VisType.editor key.

There should be no functional change introduced by this PR.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@timroes timroes requested review from sulemanof, a team and stratoula October 8, 2020 14:24
@timroes timroes added chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0 labels Oct 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

sulemanof
sulemanof approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@sulemanof sulemanof left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @timroes !

src/plugins/visualize/public/application/utils/use/use_saved_vis_instance.ts
@@ -44,7 +44,7 @@ export const useSavedVisInstance = (
savedVisInstance?: SavedVisInstance;
visEditorController?: IEditorController;
}>({});
const visEditorRef = useRef<HTMLDivElement>(null);
const visEditorRef = useRef<HTMLDivElement | null>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Docs note:

if you need the result of useRef to be directly mutable, include | null in the type of the generic argument.

That means, visEditorRef.current is mutable with such a type.
If you'll get rid of union type and leave only the HTMLDivElement, this will still save possible null value, but in that case visEditorRef.current is a read-only property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's exactly why I added it here :) Since the file now behaves slightly different I need to make sure visEditorRef has a value in the tests, since otherwise it does not run through the initialization phase, so I need it to be a MutualRef object in that case. This seemed for me to be the easiest way.

timroes
timroes commented Oct 9, 2020
View reviewed changes
src/plugins/visualize/public/application/utils/use/use_vis_byvalue.ts
@@ -41,7 +41,7 @@ export const useVisByValue = (
useEffect(() => {
const { chrome } = services;
const getVisInstance = async () => {
if (!valueInput || loaded.current) {
if (!valueInput || loaded.current || !visEditorRef.current) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ThomThomson Could you check if this is still working as expected? This should not break things theoretically, because the Editor would anyway not have worked if there was no HTML element passed into, just want to double check.

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

stratoula
stratoula approved these changes Oct 12, 2020
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.

LGTM, thanx Tim 🙂

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
visualizations 135 134 -1
visualize 206 205 -1
total -2

async chunks size

id before after diff
visualize 264.2KB 264.2KB +23.0B

page load bundle size

id before after diff
visualizations 273.5KB 273.0KB -452.0B
visualize 41.5KB 41.3KB -172.0B
total -624.0B

History

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

@timroes timroes merged commit 162cf2e into elastic:master Oct 13, 2020
@timroes timroes deleted the ts/improve-viseditor-typing branch October 13, 2020 08:59
@timroes timroes mentioned this pull request Oct 13, 2020
Merged
timroes pushed a commit to timroes/kibana that referenced this pull request Oct 13, 2020
@kibanamachine @timroes
Improve vis editor typings (elastic#80004)
855c647 
* Improve vis editor typings

* Move VisEditorContructor to visualize

* Fix tests

* Fix typings

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 13, 2020
@gmmorris
Merge remote-tracking branch 'upstream/master' into task-manager/health
4f37202 
* upstream/master: (34 commits)
  Improve vis editor typings (elastic#80004)
  Line Visualization improper scaling can result in gaps elastic#79663 (elastic#80135)
  [Security Solution][Timeline] Fix SelectableTimeline search (elastic#80128)
  move field_mapping util to saved_objects plugin (elastic#79918)
  [ML] Datagrid: Ensure column content with 'boolean' schema is not capitalized (elastic#80041)
  [CI] Correctly resolve repository root for JUnit reports (elastic#80226)
  [Ingest Manager] Fix package upgrade breaking after first rollover before new data has arrived (elastic#79887)
  [Security Solution] Fix positioning of Kibana banners list inside Sec… (elastic#80124)
  add missing await to fix test (elastic#80202)
  Revert test data changed in previous commit. (elastic#79479)
  [Security Solution] [Sourcerer] Jest beef up (elastic#79907)
  Re-enable transaction duration alert story (elastic#80187)
  [npm] remove canvas dep (elastic#80185)
  [DOCS] Redirects ILM docs to Elasticsearch reference (elastic#79798)
  [APM] Catch health status error from ML (elastic#80131)
  Fix layout and remove title for add alert popover. (elastic#77633)
  [Discover] Loading spinner cleanup (elastic#79819)
  [Security Solution] [Resolver] Remove related events api (elastic#79036)
  [Ingest Manager] Remove fields from index pattern during package uninstall (elastic#80082)
  do not refetch license if signature header absents from a response (elastic#79645)
  ...
timroes pushed a commit that referenced this pull request Oct 13, 2020
@kibanamachine
Improve vis editor typings (#80004) (#80300)
98eb9cd 
* Improve vis editor typings

* Move VisEditorContructor to visualize

* Fix tests

* Fix typings

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@sulemanof sulemanof sulemanof approved these changes

Assignees
No one assigned
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@timroes @elasticmachine @stratoula @kibanamachine @sulemanof