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] Make operation order more clear to users #48305

Merged
merged 6 commits into from
Oct 16, 2019

Conversation

wylieconlon
Copy link
Contributor

@wylieconlon wylieconlon commented Oct 15, 2019

Closes #47281

Lens allows users to build multi-level aggregations using simplified editors, but user interviews indicated that even advanced Elasticsearch users were confused by our terminology of "Nesting."

This PR:

  • Changes from describing "Nest under" to a more clear "Group by" or "Group <a> for each <b>"
  • Prevents users from editing the hierarchy on the X axis, which was a source of confusion
  • Improves the wording on suggestions that change the hierarchy
  • Improves the form for changing the hierarchy
  • Strictly determines the order of the Data Table visualization based on the hierarchy

The operation editor is used in all charts:

When there are 2 bucketed operations we show radio buttons:

Screenshot 2019-10-15 18 15 57

This also works for dates:

Screenshot 2019-10-15 19 11 22

Editor as seen in a data table:

Screenshot 2019-10-15 17 10 56

Suggestion labeling:

Screenshot 2019-10-15 18 18 32

@wylieconlon wylieconlon added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.5.0 labels Oct 15, 2019
@wylieconlon wylieconlon requested review from chrisdavies, AlonaNadler, gchaps and a team October 15, 2019 22:18
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@chrisdavies
Copy link
Contributor

We may want to run this text by Alona. I find this pretty confusing:

@timestamp for each date
Dates for each @timestamp

Also, Overall top process.name is a bit clumsy, though it's nice that it's different at a glance. I think Top values overall is clearer.

Is "date" the xAxis field in your example? If so, it makes more sense.

@chrisdavies
Copy link
Contributor

Also, since we're re-ordering in the table (and presumably in any similar interface), maybe the multi-nesting scenario should be labeled: Group beneath? Dunno.

Copy link
Contributor

@chrisdavies chrisdavies left a comment

Choose a reason for hiding this comment

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

LGTM w/ possible tweaks. We can always make verbiage tweaks later.

return !originalOrder.includes(id);
})
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be simplified, I think:

const sortedColumns = Array.from(new Set(originalOrder.concat(layer.columns)));

>
<>
<EuiRadio
id={generator()}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with this, but is this predictive, or is it going to cause render churn? It seems totally fine to me to just specify an ID schematically here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I'm supposed to pass a string, and it'll generate consistent ids if given a string.


if (!column || !column.isBucketed || !aggColumns.length) {
return null;
}

const fieldName = hasField(column) ? column.sourceField : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same logic as above. Not a big deal. But might be something we can tidy up later.

values: { target: target.fieldName },
})
: i18n.translate('xpack.lens.indexPattern.groupingSecondDateHistogram', {
defaultMessage: 'Dates for each {target}',
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this verbiage pretty confusing. I think {field} for each {target} or something would be clearer.

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 think it improves the readability to use field names like that- especially when the editor is showing the current field in multiple places.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@wylieconlon
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@wylieconlon wylieconlon merged commit 76e1398 into elastic:master Oct 16, 2019
@wylieconlon wylieconlon deleted the lens/operation-ordering branch October 16, 2019 04:43
wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Oct 16, 2019
* [Lens] Make operation nesting more clear to users

* Improve date wording

* Update per comments
wylieconlon pushed a commit that referenced this pull request Oct 16, 2019
* [Lens] Make operation nesting more clear to users

* Improve date wording

* Update per comments
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] remove nesting switch on x-axis
3 participants