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

moved vis table to use EUI #30610

Merged
merged 29 commits into from
Feb 20, 2019
Merged

moved vis table to use EUI #30610

merged 29 commits into from
Feb 20, 2019

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Feb 10, 2019 •
edited
Loading

Rewrite a common list table components using EUI and use it for Visualizations and Dashboards apps.

@lizozom lizozom requested a review from a team as a code owner February 10, 2019 16:22
@lizozom lizozom requested a review from timroes February 10, 2019 16:23
@lizozom lizozom added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Feb 10, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

cchaos
cchaos suggested changes Feb 11, 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.

Thanks for taking this on @lizozom! I just had a couple suggestions/found some errors.

When I click on a vis item, it takes me back to the home page. It looks like the all the link href's are: href="##/visualize/edit/undefined".

src/legacy/core_plugins/kibana/public/visualize/listing/_listing.scss Show resolved Hide resolved
src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js Outdated Show resolved Hide resolved
src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js Outdated Show resolved Hide resolved
src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js Outdated Show resolved Hide resolved
src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor Author

lizozom commented Feb 12, 2019

#30803

@lizozom
Copy link
Contributor Author

lizozom commented Feb 12, 2019

retest

@lizozom
Copy link
Contributor Author

lizozom commented Feb 12, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor Author

lizozom commented Feb 14, 2019

@cchaos please review

The new Visualize UI:

image

Old Visualize UI:

image

Dashboards after refactoring (unchanged, except for pencil icon for edit and + icon for create):

image

Dashboards before:

image

@spalger
Copy link
Contributor

spalger commented Feb 14, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Feb 14, 2019

Looking good!

I would shrink the action column's width to give more space to the name and description columns on both tables.

image
image

Also, if you remove the span that surrounds the flask icon and the link, they'll be children of a flex-box allowing them to stay side by side and not have the link shift to it's own line like so:

image

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2019

The more I look at it, the less I think the "Experimental" identifier (flask) should be coupled with the visualization name. It can cause misunderstanding that the flask is actually part of the name and really, it should indicate that the chart type is experimental.

Looking at the chart selector, we've moved away from using the flask and use the EuiBetaBadge with an "E" to indicate experimental.

screen shot 2019-02-19 at 09 28 29 am

Let's do the same for the chart listing table and move the experimental indicator to the type column and use the EuiBetaBadge the same way to the right:

screen shot 2019-02-19 at 09 52 35 am

Side note: That Experimental explanation needs some work. @gchaps Any suggestions?

@gchaps
Copy link
Contributor

gchaps commented Feb 19, 2019

Here is a suggestion. The language is a condensed version of the text that we use for the experimental tag in the documentation:

This visualization might be changed or removed in a future release and is not subject to the support SLA.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor Author

lizozom commented Feb 20, 2019 •
edited
Loading

@cchaos @gchaps looks much better this way

image

I also updated the new visualization menu badge to use the same text.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor Author

lizozom commented Feb 20, 2019

retest flaky

@lizozom
Copy link
Contributor Author

lizozom commented Feb 20, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom
Copy link
Contributor Author

lizozom commented Feb 20, 2019

retest

cchaos
cchaos approved these changes Feb 20, 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.

Fantastic! Looks great, thank you. Sorry for all the extra work.

Just had one comment about pixel values in SASS but other than that, LGTM!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom lizozom merged commit dc654e4 into elastic:master Feb 20, 2019
@lizozom lizozom mentioned this pull request Feb 20, 2019
Merged
lizozom added a commit to lizozom/kibana that referenced this pull request Feb 20, 2019
@lizozom
moved vis table to use EUI (elastic#30610)
d377d75 
* Create base class for table list view, using EuiInMemoryTable.
* Reimplemented dashboards and visualization listing views using table list view component.
* Added tests for visualizations page
* Updated experimental visualization badge and tooltip
@lizozom lizozom added v7.2.0 and removed v7.0.0 labels Feb 20, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

lizozom added a commit that referenced this pull request Feb 20, 2019
@lizozom
moved vis table to use EUI (#30610) (#31581)
f2a0d31 
* Create base class for table list view, using EuiInMemoryTable.
* Reimplemented dashboards and visualization listing views using table list view component.
* Added tests for visualizations page
* Updated experimental visualization badge and tooltip
@timroes timroes mentioned this pull request Feb 20, 2019
Closed
3 tasks
@kobelb kobelb mentioned this pull request Feb 20, 2019
Merged
@markov00 markov00 mentioned this pull request Jul 15, 2019
Closed
@cchaos cchaos added EUI and removed Feature:EUI labels Aug 18, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-design (EUI)

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

@cchaos cchaos cchaos approved these changes

@flash1293 flash1293 flash1293 approved these changes

@timroes timroes Awaiting requested review from timroes

Assignees
No one assigned
Labels
EUI release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.2.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lizozom @elasticmachine @spalger @cchaos @gchaps @flash1293 @timroes