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

[Lens] Fix missing formatting bug in "break down by" #63288

Merged
merged 3 commits into from
Apr 14, 2020
Merged

[Lens] Fix missing formatting bug in "break down by" #63288

merged 3 commits into from
Apr 14, 2020

Conversation

wylieconlon
Copy link
Contributor

Fixes bug where the "break down by" values would not get formatted, as reported by @flash1293 in #61968. This is caused by a change to the name function we provide to the chart, which was not handling this case.

After fix:
Screenshot 2020-04-10 16 19 05

I've ensured that this change doesn't affect any of the existing visualizations by testing the following scenarios, and adding unit tests for this:

  • Basic XY chart with one Y
  • Basic XY chart with two Y
  • Split series chart with one Y, no special formatting
  • Split series chart with two Y, no special formatting
  • Split series chart with one Y, formatted
  • Split series chart with two Y, formatted

One of the side effects of this change is related to duration fields specifically, because each of these values represents a number in milliseconds that is displayed the same:

Screenshot 2020-04-10 16 21 42

Fixes #61968

Checklist

@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.7.0 v7.8.0 labels Apr 10, 2020
@wylieconlon wylieconlon requested review from timroes, mbondyra, nickofthyme and a team April 10, 2020 20:23
@elasticmachine
Copy link
Contributor

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

@wylieconlon
Copy link
Contributor Author

@elasticmachine merge upstream

nickofthyme
nickofthyme approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Code changes look good to me with the exception of one minor comment.

Verified issue (#61968) is resolved. Specifically checked the following on Chrome and Firefox.

Using break down by date histogram. Formatting is now correct

Image 2020-04-13 at 10 44 13 AM

Using break down by with a numeric value. Formatting is now correct.

Image 2020-04-13 at 10 51 59 AM

Using multiple y values with break down by

image

x-pack/legacy/plugins/lens/public/xy_visualization/xy_expression.tsx Outdated
if (i === 0 && splitHint) {
return formatFactory(splitHint).convert(key);
}
return columnToLabelMap[key] ?? key;
Copy link
Contributor

Choose a reason for hiding this comment

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

could columnToLabelMap[key] ever be ''? In which case this would join an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the empty string is possible if the user deletes the default label that we apply. It could be more correct in that case to use the logic columnLabelMap[key] || key which will fall back to showing the key.

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've now updated this logic so that it's impossible to see the UUIDs that we use internally

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

Tested on Chrome, LGTM

@wylieconlon wylieconlon added v7.7.1 and removed v7.7.0 labels Apr 14, 2020
@wylieconlon wylieconlon merged commit 7cd746b into elastic:master Apr 14, 2020
@wylieconlon wylieconlon deleted the lens/fix-breakdown-formatting branch April 14, 2020 21:20
This was referenced Apr 14, 2020
Merged
Merged
wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Apr 14, 2020
@elasticmachine @wylieconlon
[Lens] Fix missing formatting bug in "break down by" (elastic#63288)
a47b5fe 
* [Lens] Fix missing formatting bug in "break down by"

* Stop showing UUIDs, make logic more explicit

Co-authored-by: Elastic Machine <[email protected]>
timroes pushed a commit that referenced this pull request Apr 15, 2020
@elasticmachine
[Lens] Fix missing formatting bug in "break down by" (#63288) (#63527)
0da8f78 
* [Lens] Fix missing formatting bug in "break down by"

* Stop showing UUIDs, make logic more explicit

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 15, 2020
@gmmorris
Merge branch 'master' into alerting/deleted-alert-error
636ff99 
* master: (29 commits)
  Add test:jest_integration npm script (elastic#62938)
  [data.search.aggs] Remove service getters from agg types (AggConfig part) (elastic#62548)
  [Discover] Fix broken setting of bucketInterval (elastic#62939)
  Disable adding conditions when in alert management context. (elastic#63514)
  [Alerting] fixes to allow pre-configured actions to be executed (elastic#63432)
  adding useMemo (elastic#63504)
  [Maps] fix double fetch when filter pill is added (elastic#63024)
  [Lens] Fix missing formatting bug in "break down by" (elastic#63288)
  [SIEM] [Cases] Removed double pasted line (elastic#63507)
  [Reporting] Improve functional test steps (elastic#63259)
  [SIEM][CASE] Tests for server's configuration API (elastic#63099)
  [SIEM] [Cases] Case container unit tests (elastic#63376)
  [ML] Improving parsing of large uploaded files (elastic#62970)
  [ML] Listing global calendars on the job management page (elastic#63124)
  [Ingest][Endpoint] Add Ingest rest api response types for use in Endpoint (elastic#63373)
  Add help text to form fields (elastic#63165)
  [ML] Converts utils Mocha tests to Jest (elastic#63132)
  [Metrics UI] Refactor With* containers to hooks (elastic#59503)
  [NP] Migrate logstash server side code to NP (elastic#63135)
  Clicking cancel in saved query save modal doesn't close it (elastic#62774)
  ...
wayneseymour pushed a commit that referenced this pull request Apr 15, 2020
@elasticmachine @wayneseymour
[Lens] Fix missing formatting bug in "break down by" (#63288)
ccf54a4 
* [Lens] Fix missing formatting bug in "break down by"

* Stop showing UUIDs, make logic more explicit

Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 16, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

19 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

wylieconlon pushed a commit that referenced this pull request May 13, 2020
@elasticmachine
[Lens] Fix missing formatting bug in "break down by" (#63288) (#63528)
b83498c 
* [Lens] Fix missing formatting bug in "break down by"

* Stop showing UUIDs, make logic more explicit

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickofthyme nickofthyme nickofthyme approved these changes

@mbondyra mbondyra mbondyra approved these changes

@timroes timroes Awaiting requested review from timroes

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

Successfully merging this pull request may close these issues.

Lens: "Break down by" field is not formatted
5 participants
@wylieconlon @elasticmachine @kibanamachine @mbondyra @nickofthyme