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

reactifying visualization #16425

Merged
merged 12 commits into from
Jun 27, 2018
Merged

reactifying visualization #16425

merged 12 commits into from
Jun 27, 2018

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jan 31, 2018 •
edited
Loading

moves from angular to react

this is based on top of #16249 and requires the new inspector panel (spy panel replacement) @timroes is working on before it can be merged

huge thanks to @timroes for helping me out with this 👍

@ppisljar ppisljar added review WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 v6.3.0 labels Jan 31, 2018
@ppisljar ppisljar changed the title reactifying <visualization> reactifying visualization Jan 31, 2018
@thomasneirynck
Copy link
Contributor

I removed the 6.3 label for now, it's unlikely this'll make it I think.

@thomasneirynck thomasneirynck removed their request for review April 16, 2018 13:59
@elastic elastic deleted a comment from elasticmachine Jun 6, 2018
@elastic elastic deleted a comment from elasticmachine Jun 6, 2018
@elastic elastic deleted a comment from elasticmachine Jun 6, 2018
@elastic elastic deleted a comment from elasticmachine Jun 6, 2018
@elastic elastic deleted a comment from elasticmachine Jun 6, 2018
@elastic elastic deleted a comment from elasticmachine Jun 6, 2018
@ppisljar ppisljar force-pushed the fix/visualizationReact branch 3 times, most recently from 997af7c to cf5b74b Compare June 7, 2018 15:47
@ppisljar
Copy link
Member Author

inspector needs to be merged before this can proceed

@ppisljar ppisljar removed the blocked label Jun 20, 2018
@ppisljar ppisljar removed the WIP Work in progress label Jun 20, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

timroes
timroes reviewed Jun 27, 2018
View reviewed changes
src/ui/public/visualize/loader/index.js Outdated
@@ -17,4 +17,5 @@
* under the License.
*/

export * from './loader';
export * from './visualize_loader';
export * from './visualization_loader';
Copy link
Contributor

@timroes timroes Jun 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Since we don't want to make this a public API that third party plugins consume, could we perhaps not export this via the index.js file, so that we rather import from ui/visualize/loader/visualization_loader, and not having it in the same file that users import the visualize loader from, which we consider public? Maybe even rename that file to _visualization_loader just to make clear, that's nothing you should import outside of visualization code?

timroes
timroes suggested changes Jun 27, 2018
View reviewed changes
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Some minor points.

I would really prefer if we make visualization loader more "invisible" so people not accidentally start using it.

src/ui/public/visualize/visualization.js Outdated
this.forceUpdate();
};

static getDerivedStateFromProps(props, prevState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we potentially move the static method to top (or less preferred by me: bottom), to not have it sitting somewhere in the middle?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think it makes more sense to order the methods in lifecycle order, but whatever ... this is a real nity pick by the way :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I know. That's why I didn't explicitly state it - I though you'll figure it out anyway :D

src/ui/public/visualize/loader/__tests__/visualization_loader.js Outdated
}));
setupAndTeardownInjectorStub();

describe('', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing title

src/ui/public/visualize/visualization.test.js
}

render() {
renderPromise = new Promise((resolve) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could actually shorten this method to:

async render() {
  this.el.innerText = this.vis.params.markdown;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

but then we won't be able to wait for this renderPromise to be resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true. Sorry haven't noticed that! Forget that comment.

@ppisljar
Copy link
Member Author

@timroes we re not hidding any other code we don't want 3rd party developers to use ? i think that's a bad concept.

@ppisljar ppisljar merged commit 58cd7c8 into elastic:master Jun 27, 2018
@ppisljar ppisljar mentioned this pull request Jun 27, 2018
Merged
ppisljar added a commit to ppisljar/kibana that referenced this pull request Jun 27, 2018
@ppisljar
reactifying visualization (elastic#16425)
c5accc7
@timroes
Copy link
Contributor

timroes commented Jun 27, 2018

Yeah I know we don't do that in a lot of places (though we have some places where we use the _filename for private files), I am just a bit worried, since the visualize loader is actually one of the only APIs we really consider an API at the moment, and that it might just tend people start using the other (also since it's not obvious what visualize vs visualization is). But I also don't have too strong feelings, so go with your best judgment there :)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

ppisljar added a commit that referenced this pull request Jun 27, 2018
@ppisljar
reactifying visualization (#16425) (#20270)
fd0b047
@ppisljar ppisljar deleted the fix/visualizationReact branch June 27, 2018 15:35
mattapperson pushed a commit that referenced this pull request Jun 28, 2018
@ppisljar @mattapperson
reactifying visualization (#16425)
d783ce2
@ppisljar ppisljar restored the fix/visualizationReact branch September 26, 2018 10:06
@timroes timroes mentioned this pull request Dec 12, 2018
Closed
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timroes timroes timroes requested changes

@markov00 markov00 markov00 approved these changes

Assignees
No one assigned
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ppisljar @thomasneirynck @elasticmachine @timroes @markov00