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

[Vis: Default editor] EUIficate Panel Setting tab #42828

Merged
merged 16 commits into from
Aug 14, 2019
Merged

[Vis: Default editor] EUIficate Panel Setting tab #42828

merged 16 commits into from
Aug 14, 2019

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Aug 7, 2019 •
edited
Loading

Summary

A part of #38273.
EUIfication of the point-series and grid templates from the Panel Setting tab in the Area/Bar/Line vis.

Before After

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@maryia-lapata maryia-lapata added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.4.0 labels Aug 7, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine

This comment has been minimized.

Sign in to view
@maryia-lapata maryia-lapata marked this pull request as ready for review August 8, 2019 13:27
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Aug 8, 2019

Couple UI suggestions for you:

Also, I was playing in master with the "Show x-axis" and it doesn't seem to be doing anything. Is there a bug somewhere?

@timroes timroes mentioned this pull request Aug 8, 2019
Merged
7 tasks
@maryia-lapata
Copy link
Contributor Author

@cchaos I tried with [Flights] Delay Type vis, it works.

Aug-08-2019 18-43-41

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Aug 8, 2019

Ah yeah it does work for [Flights] Delay Type but it isn't working for [Flights] Delay Buckets. Can you find out what the differences are between those two as to why one works and the other doesn't? I'd like to be able to disable that switch if there's a config that's not allowing it.

sulemanof
sulemanof reviewed Aug 9, 2019
View reviewed changes
src/legacy/core_plugins/kbn_vislib_vis_types/public/utils/legend_positions.ts Outdated
}
};
});
export { legendPositions };
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add legendPositions into editorConfig.collections to be in consistency with other configs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

sulemanof
sulemanof reviewed Aug 9, 2019
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.

Overall, LGTM 👌

@maryia-lapata
Copy link
Contributor Author

Ah yeah it does work for [Flights] Delay Type but it isn't working for [Flights] Delay Buckets. Can you find out what the differences are between those two as to why one works and the other doesn't? I'd like to be able to disable that switch if there's a config that's not allowing it.

@cchaos so far I noticed that categoryLines (Show x-axis lines) isn't applied when vis has Histogram agg. It's true for Area, Horizontal and Vertical bar and Line.
I'll investigate further.

@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@maryia-lapata
Copy link
Contributor Author

@cchaos I discussed the "Show x-axis" behavior with @timroes and @markov00. We agreed to disable that config for vis that has Histogram agg. The changes has been added.

We should revert this changes when elastic-chart will be applied.

cchaos
cchaos reviewed Aug 13, 2019
View reviewed changes
src/legacy/core_plugins/kbn_vislib_vis_types/public/controls/point_series/grid_options.tsx
@@ -59,6 +70,7 @@ function GridOptions({ stateParams, setValue }: VisOptionsProps<BasicVislibParam
</EuiTitle>

<SwitchOption
disabled={hasHistogramAgg}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it then also possible to add a tooltip saying such? Like tip={hasHistogramAgg ? 'X axis lines can't show for histograms' : undefined}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Added.

image

@elasticmachine

This comment has been minimized.

Sign in to view
@maryia-lapata maryia-lapata mentioned this pull request Aug 14, 2019
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

flash1293
flash1293 approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

There is one spacer element that could be removed IMHO, rest LGTM.

src/legacy/core_plugins/kbn_vislib_vis_types/public/editors/point_series.tsx Outdated
/>
)}

<EuiSpacer size="s" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this spacer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it looks fine without it:

I removed it.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

cchaos
cchaos approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thx!

@maryia-lapata maryia-lapata merged commit a892d58 into elastic:master Aug 14, 2019
@maryia-lapata maryia-lapata deleted the point-series branch August 14, 2019 14:52
@maryia-lapata maryia-lapata mentioned this pull request Aug 14, 2019
Merged
maryia-lapata added a commit that referenced this pull request Aug 15, 2019
@maryia-lapata
[Vis: Default editor] EUIficate Panel Setting tab (#42828) (#43277)
f5adc2c 
* EUIficate pointe-series and grid

* Apply TS

* Show grid on a panel

* Remove extra space

* Add TS

* Use BasicOptions

* Adjust func test

* Add dataTestSubj prop to SelectOption

* Use id instead of data-sest-subj

* Disable show x-axis lines when there is histogram agg

* Add tooltip for disabled 'Show x-axis lines' config

* Remove extra space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sulemanof sulemanof sulemanof left review comments

@flash1293 flash1293 flash1293 approved these changes

@cchaos cchaos cchaos approved these changes

@wylieconlon wylieconlon Awaiting requested review from wylieconlon

Assignees
No one assigned
Labels
Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maryia-lapata @elasticmachine @cchaos @flash1293 @sulemanof