[NP] Vis Default Editor plugin #62475

sulemanof · 2020-04-03T16:05:56Z

Summary

Moves the vis_default_editor plugin into new platform.

Changes mostly contains files movements and paths changes.
Apart from that, since eslint ignores were removed, there are some changes related to exhaustive deps arrays in useCallback, useEffect functions

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

…itor

sulemanof · 2020-04-06T13:10:28Z

.eslintrc.js

-    {
-      files: ['src/legacy/core_plugins/expressions/**/*.{js,ts,tsx}'],
-      rules: {
-        'react-hooks/exhaustive-deps': 'off',
-      },
-    },
-    {
-      files: [
-        'src/legacy/core_plugins/vis_default_editor/public/components/controls/**/*.{ts,tsx}',
-      ],
-      rules: {
-        'react-hooks/exhaustive-deps': 'off',
-      },
-    },
-    {
-      files: ['src/legacy/ui/public/vis/**/*.{js,ts,tsx}'],
-      rules: {
-        'react-hooks/exhaustive-deps': 'off',
-      },
-    },


Don't need these ignores anymore 🎉

sulemanof · 2020-04-06T13:13:33Z

src/plugins/vis_default_editor/public/components/controls/has_extended_bounds.tsx


  return (
    <SwitchParamEditor
+      {...props}


This was moved upper since props has a disabled field which overrides local disabled variable.
A separate fix could be prepared for this into 7.7

elasticmachine · 2020-04-06T14:23:12Z

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

sulemanof · 2020-04-07T08:17:50Z

@elasticmachine merge upstream

flash1293

The move to the new platform looks good to me, but I'm not sure about some of the hooks fixes in a lot of files (see comment in code).

flash1293 · 2020-04-07T08:36:56Z

src/plugins/vis_default_editor/public/components/controls/filters.tsx

-    // set parsed values into model after initialization
-    setValue(filters.map(filter => omit({ ...filter, input: filter.input }, 'id')));
-  }, []);
+    if (firstRender.current) {


So this firstRender workaround is basically an alternative to disabling linting like in https://github.com/elastic/kibana/pull/62475/files?diff=unified&w=1#diff-68df3519b81dd4040af6faf4820bd3b7R101 , right?

Why are you using both ways in different places? Subjectively I prefer the comment disabling linting for now, this seems dirtier to me.

Ideally I think we should move these "initial setups" out of react components completely, but that's definitely out of scope for this PR.

You are right, it looks not really in place here and breaks up the React Hooks essentials,
but I wouldn't also use the eslint disabling (the one you mentioned was just accidentally missed, my fault).
I'm not sure how soon we'll be able to refactor this code due to we have a list of blockers and to-dos first to make the default editor more pure and meet meet all of React guidelines (here I mean agg configs restructure, agg types, etc).
In that case I would consider using more explicit helper - useMount from react-use , so this will look like :

import { useMount } from 'react-use'; ... useMount(() => { // set parsed values into model after initialization setValue(filters.map(filter => omit({ ...filter, input: filter.input }, 'id'))); });

What do you think ?

+1 for this, as we are using react-use in a bunch of places it sounds like a solution that achieves the same hiding the workaround somewhere else.

…itor

…nto np/vis_default_editor

kibanamachine · 2020-04-07T14:49:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3003804

History

💔 Build #39178 failed 17612d2
💔 Build #38918 failed 4de5a19
💔 Build #38465 failed 5f10f36

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

cchaos

SASS file moves are ok.

flash1293

Tested the places where hooks got changed non-trivially, couldn't find any breakages. LGTM

* Move the default_editor to NP * Fix paths * Import styles through the visualize * Other fixes * Fix ip_ranges exhaustive-deps array * Fix filters and extend bounds * Other fixes * Fix date_ranges tests * Use useMount on first render Co-authored-by: Elastic Machine <[email protected]>

* Move the default_editor to NP * Fix paths * Import styles through the visualize * Other fixes * Fix ip_ranges exhaustive-deps array * Fix filters and extend bounds * Other fixes * Fix date_ranges tests * Use useMount on first render Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

* Move the default_editor to NP * Fix paths * Import styles through the visualize * Other fixes * Fix ip_ranges exhaustive-deps array * Fix filters and extend bounds * Other fixes * Fix date_ranges tests * Use useMount on first render Co-authored-by: Elastic Machine <[email protected]>

sulemanof added 10 commits April 2, 2020 15:56

Move the default_editor to NP

dd7fb2a

Fix paths

9bbe16d

Merge remote-tracking branch 'upstream/master' into np/vis_default_ed…

ed59024

…itor

Import styles through the visualize

51e775f

Other fixes

5f10f36

Merge remote-tracking branch 'upstream/master' into np/vis_default_ed…

de82d3a

…itor

Fix ip_ranges exhaustive-deps array

bca5eab

Fix filters and extend bounds

ccdf5a7

Other fixes

90fe035

Merge remote-tracking branch 'upstream/master' into np/vis_default_ed…

e82b931

…itor

sulemanof commented Apr 6, 2020

View reviewed changes

flash1293 mentioned this pull request Apr 6, 2020

Meta: Kibana app NP cutover #60097

Closed

69 tasks

sulemanof added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8 v8.0.0 labels Apr 6, 2020

Fix date_ranges tests

4de5a19

sulemanof marked this pull request as ready for review April 6, 2020 15:54

sulemanof requested a review from a team April 6, 2020 15:54

sulemanof requested a review from a team as a code owner April 6, 2020 15:54

rayafratkina added v7.8.0 and removed v7.8 labels Apr 6, 2020

Merge branch 'master' into np/vis_default_editor

17612d2

flash1293 reviewed Apr 7, 2020

View reviewed changes

sulemanof added 2 commits April 7, 2020 14:51

Use useMount on first render

e0a7897

Merge remote-tracking branch 'upstream/master' into np/vis_default_ed…

b6805f3

…itor

Merge branch 'np/vis_default_editor' of github.com:sulemanof/kibana i…

3003804

…nto np/vis_default_editor

cchaos approved these changes Apr 7, 2020

View reviewed changes

sulemanof mentioned this pull request Apr 8, 2020

[7.7] Fix disabled switches in the editor #62911

Merged

7 tasks

flash1293 approved these changes Apr 8, 2020

View reviewed changes

sulemanof merged commit 314c4f7 into elastic:master Apr 8, 2020

sulemanof deleted the np/vis_default_editor branch April 8, 2020 09:00

sulemanof mentioned this pull request Apr 8, 2020

[7.x] [NP] Vis Default Editor plugin (#62475) #62918

Merged

sulemanof added the backported label Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NP] Vis Default Editor plugin #62475

[NP] Vis Default Editor plugin #62475

sulemanof commented Apr 3, 2020 •

edited

Loading

sulemanof Apr 6, 2020

sulemanof Apr 6, 2020

elasticmachine commented Apr 6, 2020

sulemanof commented Apr 7, 2020

flash1293 left a comment

flash1293 Apr 7, 2020 •

edited

Loading

sulemanof Apr 7, 2020

flash1293 Apr 7, 2020

sulemanof Apr 7, 2020

kibanamachine commented Apr 7, 2020

cchaos left a comment

flash1293 left a comment

[NP] Vis Default Editor plugin #62475

[NP] Vis Default Editor plugin #62475

Conversation

sulemanof commented Apr 3, 2020 • edited Loading

Summary

Checklist

For maintainers

sulemanof Apr 6, 2020

Choose a reason for hiding this comment

sulemanof Apr 6, 2020

Choose a reason for hiding this comment

elasticmachine commented Apr 6, 2020

sulemanof commented Apr 7, 2020

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

sulemanof Apr 7, 2020

Choose a reason for hiding this comment

flash1293 Apr 7, 2020

Choose a reason for hiding this comment

sulemanof Apr 7, 2020

Choose a reason for hiding this comment

kibanamachine commented Apr 7, 2020

💚 Build Succeeded

History

cchaos left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

sulemanof commented Apr 3, 2020 •

edited

Loading

flash1293 Apr 7, 2020 •

edited

Loading