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

New visualization type selection #23833

Merged
merged 32 commits into from
Nov 22, 2018
Merged

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Oct 4, 2018

Summary

Fixes #17024

Implement the new visualization type selection in an EUI modal.

screenshot-20181004-175651

TODOs

Checklist

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

For maintainers

Dev Docs

Visualization Categories

With introduction of the new visualization type selection we removed the category key from visualizations. All you need to do is to remove the category key from your visualization registration.

If you used CATEGORY.HIDDEN as a value to hide the type from the visualization wizard, just use the new hidden: true property instead.

@timroes timroes added release_note:enhancement WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 4, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes timroes added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Oct 15, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes timroes force-pushed the eui-vis-type-selection branch from 5b604a9 to dbb6f01 Compare October 15, 2018 14:41
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor Author

timroes commented Oct 16, 2018

Jenkins, test this - browser didn't start

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor Author

timroes commented Nov 7, 2018

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor Author

timroes commented Nov 19, 2018

Jenkins, test this - CI failure

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

Looks really good. I did notice that some of the "lab" visualizations have not been changed to "experimental". Also, here is a PR with some design fixes: timroes#4

<EuiModalHeaderTitle>
<FormattedMessage
id="kbn.visualize.newVisWizard.title"
defaultMessage="New Visualizations"
Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is this should not be plural "Visualizations" because it can only create one new one at a time?

@timroes
Copy link
Contributor Author

timroes commented Nov 20, 2018

@cchaos Thanks for the PR. Will incorporate those changes. The lab removal has been done in a separate PR that was just merged yesterday, and I didn't merge master into that PR yet, so that's expected behavior, that there are still lab visualizations around :-)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Tested locally and this looks great! No major concerns on my end; just added a few nits.

@@ -0,0 +1,99 @@
.visNewVisDialog {
background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='166' height='157' viewBox='0 0 166 157'%3E%3Cdefs%3E%3ClinearGradient id='untitled-2-a' x1='0%25' y1='0%25' y2='100%25'%3E%3Cstop offset='0%25' stop-color='%23FFF' stop-opacity='.2'/%3E%3Cstop offset='100%25' stop-opacity='0'/%3E%3C/linearGradient%3E%3CradialGradient id='untitled-2-b' cx='0%25' cy='0%25' r='127.787%25' fx='0%25' fy='0%25' gradientTransform='matrix(.681 .68098 -.63326 .7323 0 0)'%3E%3Cstop offset='0%25' stop-color='%23BBB' stop-opacity='.1'/%3E%3Cstop offset='100%25' stop-opacity='.5'/%3E%3C/radialGradient%3E%3ClinearGradient id='untitled-2-c' x1='0%25' y1='0%25' y2='100%25'%3E%3Cstop offset='0%25' stop-color='%23FFF' stop-opacity='.4'/%3E%3Cstop offset='100%25' stop-opacity='0'/%3E%3C/linearGradient%3E%3CradialGradient id='untitled-2-d' cx='0%25' cy='0%25' r='148.851%25' fx='0%25' fy='0%25' gradientTransform='matrix(.6718 .67182 -.74072 .60932 0 0)'%3E%3Cstop offset='0%25' stop-color='%23FFF' stop-opacity='.101'/%3E%3Cstop offset='100%25' stop-opacity='.15'/%3E%3C/radialGradient%3E%3CradialGradient id='untitled-2-e' cx='0%25' cy='0%25' r='127.349%25' fx='0%25' fy='0%25' gradientTransform='matrix(.68331 .68332 -.73013 .63951 0 0)'%3E%3Cstop offset='0%25' stop-color='%23BBB' stop-opacity='.1'/%3E%3Cstop offset='100%25' stop-opacity='.5'/%3E%3C/radialGradient%3E%3C/defs%3E%3Cg opacity='.5' fill='none' fill-rule='evenodd' transform='matrix(-1 0 0 1 166 0)'%3E%3Cg opacity='.65' transform='matrix(0 -1 -1 0 146 157)'%3E%3Cpolygon fill='%23DD0A73' points='0 0 157 146 0 146' opacity='.418'/%3E%3Cpolygon fill='url(%23untitled-2-a)' points='0 0 157 146 0 146' style='mix-blend-mode:overlay'/%3E%3Cpolygon fill='url(%23untitled-2-b)' points='0 0 157 146 0 146' opacity='.618' style='mix-blend-mode:overlay'/%3E%3C/g%3E%3Cg opacity='.65' transform='translate(88 71)'%3E%3Cpath fill='%23017F75' d='M0,86 L78,86 C74.2038079,48.730962 43.6293886,16.7871605 0,0 L0,86 Z' opacity='.409'/%3E%3Cpath fill='url(%23untitled-2-c)' d='M0,86 L78,86 C74.2038079,48.730962 43.6293886,16.7871605 0,0 L0,86 Z' style='mix-blend-mode:overlay'/%3E%3Cpath fill='url(%23untitled-2-d)' d='M0,86 L78,86 C74.2038079,48.730962 43.6293886,16.7871605 0,0 L0,86 Z' opacity='.663' style='mix-blend-mode:overlay'/%3E%3C/g%3E%3Cg opacity='.15' transform='translate(73 79)'%3E%3Cpolygon fill='%23353535' points='0 0 73 78 0 78' opacity='.38'/%3E%3Cpolygon fill='url(%23untitled-2-a)' points='0 0 73 78 0 78' style='mix-blend-mode:overlay'/%3E%3Cpolygon fill='url(%23untitled-2-e)' points='0 0 73 78 0 78' style='mix-blend-mode:overlay'/%3E%3C/g%3E%3C/g%3E%3C/svg%3E%0A");
Copy link
Member

Choose a reason for hiding this comment

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

I assume the fact that you're inlining here means webpack isn't set up to import svg files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap it isn't unfortunately, already pinged the operations team regarding that. Actually webpack is not handling the SASS internals at all.

}

&::before {
top: -$euiSizeXXL + 2px; // Account for search field dropshadow
Copy link
Member

Choose a reason for hiding this comment

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

nit: The 2px value feels a little like a magic number, is it worth making it a variable? Or does EUI provide it as a variable?

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, if @cchaos already reviewed, I'm not really worried about it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually she wrote that :-) It's coming in from her change PR.

</EuiFlexItem>
<EuiFlexItem className="visNewVisDialog__description" grow={false}>
{highlightedType && <VisHelpText visType={highlightedType} />}
{!highlightedType && (
Copy link
Member

Choose a reason for hiding this comment

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

nit: could save repetition by using a ternary here

{highlightedType ? (
    <foo />
  ) : (
    <bar />
)}

{!visType.image &&
!visType.legacyIcon && (
<EuiIcon type={visType.icon} size="l" color="secondary" aria-hidden="true" />
)}
Copy link
Member

Choose a reason for hiding this comment

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

had to read through the conditionals a couple times here, I wonder if there's any way to clean this up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted it into it's own component with a better documentation. Also one of these path will be removed in 7.0, making this way more readable already :)

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Code LGTM. Tested on chrome osx.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes merged commit 8c14c25 into elastic:master Nov 22, 2018
@timroes timroes deleted the eui-vis-type-selection branch November 22, 2018 11:52
timroes added a commit to timroes/kibana that referenced this pull request Nov 22, 2018
* First version of new visualization selection

* Extract some components

* Remove visualization category

* Remove old wizard code

* Fix i18n ids

* Fix tests

* Fix tag cloud tests

* Fix broken test method

* Fix wrong method call

* Fix TSVB navigation in tests

* Restructure components

* Fix for lab removal

* Add tests

* Timroes/eui vis type selection (#4)

* Added background graphic from welcome screen to modal

* Fixed up responsiveness

* Change wording

* Fix test snapshot

* Create VisTypeIcon

* Implement suggestions

* Change experimental wording

* Use regular quotes for i18n engine
timroes added a commit that referenced this pull request Nov 22, 2018
* First version of new visualization selection

* Extract some components

* Remove visualization category

* Remove old wizard code

* Fix i18n ids

* Fix tests

* Fix tag cloud tests

* Fix broken test method

* Fix wrong method call

* Fix TSVB navigation in tests

* Restructure components

* Fix for lab removal

* Add tests

* Timroes/eui vis type selection (#4)

* Added background graphic from welcome screen to modal

* Fixed up responsiveness

* Change wording

* Fix test snapshot

* Create VisTypeIcon

* Implement suggestions

* Change experimental wording

* Use regular quotes for i18n engine
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Vis Editor Visualization editor issues release_note:enhancement release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Visualizations Visualization editors, elastic-charts and infrastructure v6.6.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants