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.

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

updated panel_state to ts gridData i vs id #22515

Merged
merged 3 commits into from
Aug 30, 2018
Merged

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Aug 29, 2018

  • panel_state.js migrated to panel_state.ts
  • GridData in selectors/types.ts file had id versus i - let me know if I'm misunderstanding but kept this to i instead of id to avoid confusion. In panel_state if I changed line 127 to id it ended up breaking my local setup so I believe changing this to I across the files will keep this consistent.
  • There was a test file panel_header_container.test.tsx that had id that I changed to i

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor

Build failures look related:

:00 
19:23:00 ERROR kibana failed
19:23:00       src/core_plugins/kibana/public/dashboard/reducers/embeddables.test.ts:39:28 - error TS2345: Argument of type '{ foo1: { embeddableConfig: {}; gridData: { h: number; id: string; w: number; x: number; y: numbe...' is not assignable to parameter of type 'PanelsMap'.
19:23:00         Property 'foo1' is incompatible with index signature.
19:23:00           Type '{ embeddableConfig: {}; gridData: { h: number; id: string; w: number; x: number; y: number; }; id...' is not assignable to type 'PanelState'.
19:23:00             Types of property 'gridData' are incompatible.
19:23:00               Type '{ h: number; id: string; w: number; x: number; y: number; }' is not assignable to type 'GridData'.
19:23:00                 Property 'i' is missing in type '{ h: number; id: string; w: number; x: number; y: number; }'.
19:23:00 
19:23:00       39   store.dispatch(setPanels({ foo1: panelData }));
19:23:00                                     ~~~~~~~~~~~~~~~~~~~
19:23:00 
19:23:00 
19:23:00       src/core_plugins/kibana/public/dashboard/reducers/panels.test.ts:41:31 - error TS2345: Argument of type '{ '1': { embeddableConfig: {}; gridData: { h: number; id: string; w: number; x: number; y: number...' is not assignable to parameter of type 'PanelsMap'.
19:23:00         Property ''1'' is incompatible with index signature.
19:23:00           Type '{ embeddableConfig: {}; gridData: { h: number; id: string; w: number; x: number; y: number; }; id...' is not assignable to type 'PanelState'.
19:23:00             Types of property 'gridData' are incompatible.
19:23:00               Type '{ h: number; id: string; w: number; x: number; y: number; }' is not assignable to type 'GridData'.
19:23:00                 Property 'i' is missing in type '{ h: number; id: string; w: number; x: number; y: number; }'.
19:23:00 
19:23:00       41   store.dispatch(updatePanels({ '1': originalPanelData }));
19:23:00                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~
19:23:00 
19:23:00 
19:23:00       src/core_plugins/kibana/public/dashboard/reducers/panels.test.ts:56:32 - error TS2345: Argument of type '{ gridData: { h: number; id: string; w: number; x: number; y: number; }; embeddableConfig: {}; id...' is not assignable to parameter of type 'PanelState'.
19:23:00         Types of property 'gridData' are incompatible.
19:23:00           Type '{ h: number; id: string; w: number; x: number; y: number; }' is not assignable to type 'GridData'.
19:23:00             Property 'i' is missing in type '{ h: number; id: string; w: number; x: number; y: number; }'.
19:23:00 
19:23:00       56     store.dispatch(updatePanel(newPanelData));
19:23:00                                         ~~~~~~~~~~~~
19:23:00 
19:23:00 
19:23:00       src/core_plugins/kibana/public/dashboard/reducers/panels.test.ts:63:27 - error TS2339: Property 'id' does not exist on type 'GridData'.
19:23:00 
19:23:00       63     expect(panel.gridData.id).toBe('1');
19:23:00                                    ~~
19:23:00 
19:23:00 
19:23:00       src/core_plugins/kibana/public/dashboard/reducers/panels.test.ts:73:33 - error TS2345: Argument of type '{ '1': { embeddableConfig: { columns: string[]; }; gridData: { h: number; id: string; w: number; ...' is not assignable to parameter of type 'PanelsMap'.
19:23:00         Property ''1'' is incompatible with index signature.
19:23:00           Type '{ embeddableConfig: { columns: string[]; }; gridData: { h: number; id: string; w: number; x: numb...' is not assignable to type 'PanelState'.
19:23:00             Types of property 'gridData' are incompatible.
19:23:00               Type '{ h: number; id: string; w: number; x: number; y: number; }' is not assignable to type 'GridData'.
19:23:00 
19:23:00       73     store.dispatch(updatePanels({ '1': panelData }));
19:23:00                                          ~~~~~~~~~~~~~~~~~~
19:23:00 
19:23:00 
19:23:00       src/core_plugins/kibana/public/dashboard/reducers/panels.test.ts:80:32 - error TS2345: Argument of type '{ embeddableConfig: { columns: string[]; }; gridData: { h: number; id: string; w: number; x: numb...' is not assignable to parameter of type 'PanelState'.
19:23:00         Types of property 'gridData' are incompatible.
19:23:00           Type '{ h: number; id: string; w: number; x: number; y: number; }' is not assignable to type 'GridData'.
19:23:00 
19:23:00       80     store.dispatch(updatePanel(newPanelData));
19:23:00                                         ~~~~~~~~~~~~
19:23:00 

@stacey-gammon
Copy link
Contributor

Looks great, awesome how switching to typescript can expose inconsistencies like that! I think that was a bug that it was i in some places, id in others. I think you made the right call using i, because that is what react-grid-layout package expects. Though eventually we should decouple these two things - so how we store data is not dependent on a third party library (and then we can use id which is much clearer than i!)

I think the tests just need to get migrated over to use i instead of of id then it should all work!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

LGTM! code review only. ci should have caught any issues.

@rshen91 rshen91 merged commit 11d6e53 into elastic:master Aug 30, 2018
@stacey-gammon
Copy link
Contributor

fyi, the labels I just applied should generally get applied to all these typescript PRs.

@stacey-gammon
Copy link
Contributor

Hey @rshen91 - just fyi, looks like this still needs to be backported.

stacey-gammon pushed a commit to stacey-gammon/kibana that referenced this pull request Sep 11, 2018
* updated panel state to ts gridData i vs id property

* changed gridData.id to gridData.i
stacey-gammon added a commit that referenced this pull request Sep 11, 2018
* updated panel state to ts gridData i vs id property

* changed gridData.id to gridData.i
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants