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

[Lens] Field item hovers, empty state graphic, and operation not applicable callout #46864

Merged
merged 18 commits into from
Oct 2, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 27, 2019

Waiting on text finalization and dark mode graphic

1. Better hover and focus states of field items

It also adds an info icon with tooltip explaining how to interact with the field item (click or drag).

2. Added the empty state graphic

Along with some better text.

Screen Shot 2019-09-30 at 12 27 58 PM

image

3. Fixed #46753

Changed callout to 'warning' color and changed text but could use a final pass on the text.

Screen Shot 2019-09-30 at 12 35 25 PM

Checklist

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

@gchaps
Copy link
Contributor

gchaps commented Sep 27, 2019

Warning

I'm a little confused by the text. Do any of these work?

To use this function, select a different field.

Cannot use this function with the current field.

Tooltip

How about being more specific about the data you can view?

Click to view top values. Or, drag into visualization.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@AlonaNadler
Copy link

I'm good with @gchaps suggestions "To use this function, select a different field ↖️ " can we add an arrow to the field drop-down?

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Left some suggestions on wording, they are optional. You do need to translate the <h3> text.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 30, 2019

I guess I'll go ahead and take this out of draft. I've made some text updates. Look like maybe the field item one is still in the works. But the rest of the PR can be reviewed.

@cchaos cchaos marked this pull request as ready for review September 30, 2019 16:51
@cchaos cchaos requested a review from a team September 30, 2019 16:51
@cchaos cchaos requested a review from a team as a code owner September 30, 2019 16:51
@cchaos cchaos added Team:Visualizations Visualization editors, elastic-charts and infrastructure and removed WIP Work in progress labels Sep 30, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@cchaos
Copy link
Contributor Author

cchaos commented Sep 30, 2019

@AlonaNadler I've updated the screenshots for items 2 and 3 above.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 30, 2019

Looks like I'm getting a type error on the chrome basepath stuff. Maybe a dev could help out:

    Error name:    "TypeError"
    Error message: "_chrome.default.addBasePath is not a function"

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos cchaos added the release_note:skip Skip the PR/issue when compiling release notes label Sep 30, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link

@AlonaNadler AlonaNadler left a comment

Choose a reason for hiding this comment

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

  • Can we make the popup disappear when selecting another field or clicking anywhere else in the UI
    currently
    Oct-01-2019 12-28-17
  • click to see a preview, drag to create a visualization
  • on the empty state preview panel, instead of build visualization, should we say create visualization? @gchaps
  • Message error looks much better
  • Empty state looks great

and adding info icon with tooltip
@cchaos
Copy link
Contributor Author

cchaos commented Oct 1, 2019

This PR now also contains a few text review updates

(See commit history and gDoc).

Status

Barring any failing TS types or test, this is reviewable.

@cchaos
Copy link
Contributor Author

cchaos commented Oct 1, 2019

Can we make the popup disappear when selecting another field or clicking anywhere else in the UI

This is not an issue introduced by this PR, it has to do with the drag and drop intervening in the outside click of the popover. I don't see it as critical will need a dev's attention so I'm going to defer this one.

click to see a preview, drag to create a visualization

There's been a lot of back and forth on this text in the doc. Please get with @gchaps to finalize this text together. For now I will keep what's in there since "preview" is confusing as I'd expect a "preview" of what it'd look like in the chart.

@gchaps
Copy link
Contributor

gchaps commented Oct 1, 2019

@AlonaNadler "Create a visualization" is good for the empty state.

@@ -260,15 +271,15 @@ function FieldItemPopoverContents(props: State & FieldItemProps) {
if (histogram && histogram.buckets.length && topValues && topValues.buckets.length) {
title = (
<EuiButtonGroup
buttonSize="s"
buttonSize="compressed"
Copy link
Contributor

Choose a reason for hiding this comment

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

Think this is related? The label is being cut off for "Distribution" Screenshot 2019-10-01 17 08 04

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 don't understand why your screenshots keep showing up as all caps in those buttons. What env are you in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I primarily develop in Firefox

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That last commit should fix your issue.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Tested in light and dark mode with a random base path. Only found one issue with the "Distribution" label, and that is something I can fix separately. Approving because I feel like this is safe to merge.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@snide snide merged commit 2f80cc5 into elastic:master Oct 2, 2019
@snide snide deleted the lens-field-panel-design branch October 2, 2019 13:13
snide pushed a commit to snide/kibana that referenced this pull request Oct 2, 2019
…icable callout (elastic#46864)

1. Better hover and focus states of field items
2. Added the empty state graphic
3. Fixed elastic#46753
snide added a commit that referenced this pull request Oct 2, 2019
…icable callout (#46864) (#47116)

1. Better hover and focus states of field items
2. Added the empty state graphic
3. Fixed #46753
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] function error color and text
6 participants