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] Combine chart switches into one layer chart switch #178971

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Mar 19, 2024

Summary

This PR combines the chart type selection options into a single, layer-based chart switch. This improves the user experience by streamlining the process of switching between chart types in Lens visualizations.

Fixes #163721
Screenshot 2024-03-25 at 10 46 24

Functionality:

Multilayer Charts: Switching to a compatible option (e.g., vertical bar to line) will only convert that specific layer.
Multilayer Charts: Switching to incompatible, multilayer option (e.g., horizontal to vertical) will convert all layers to the new type.
Multilayer to Single Layer: Switching to a single-layer chart type (line -> pie) will create the new chart based on the layer where you clicked the dropdown.

Benefits:

Simpler Interface: Reduces confusion by offering a single, unified way to switch chart types.
Improved Workflow: Makes it easier to customize individual layers or switch the entire visualization type.

Note: In Discover, due some architectural problems, we can only convert to compatible types so the chart switch is limited:
Screenshot 2024-03-25 at 11 19 02
Screenshot 2024-03-25 at 11 19 07

@mbondyra mbondyra added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens v8.14.0 labels Mar 19, 2024
@mbondyra mbondyra requested a review from a team as a code owner March 19, 2024 14:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@mbondyra mbondyra marked this pull request as draft March 19, 2024 14:43
@mbondyra mbondyra force-pushed the lens/chart_switch_into_one branch 4 times, most recently from 0e6387b to 0300fd8 Compare March 20, 2024 20:20
@elastic elastic deleted a comment from kibana-ci Mar 20, 2024
@mbondyra mbondyra force-pushed the lens/chart_switch_into_one branch from 0300fd8 to 3036a8a Compare March 21, 2024 10:24
@@ -345,57 +323,6 @@ describe('editor_frame', () => {
});
});

describe('switching', () => {
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 moved these to chart switch

@mbondyra mbondyra force-pushed the lens/chart_switch_into_one branch 2 times, most recently from 545e64a to 8cee117 Compare March 21, 2024 19:13
@mbondyra mbondyra force-pushed the lens/chart_switch_into_one branch from bf5a667 to ddfada9 Compare March 25, 2024 09:36
@mbondyra mbondyra marked this pull request as ready for review March 25, 2024 10:19
@mbondyra mbondyra requested a review from a team as a code owner March 25, 2024 10:19
@mbondyra mbondyra changed the title [Lens] consolidate chart switch into one layer chart switch [Lens] Combine chart switches into one layer chart switch Mar 25, 2024
@mbondyra mbondyra marked this pull request as draft March 25, 2024 10:20
@mbondyra mbondyra force-pushed the lens/chart_switch_into_one branch from ddfada9 to 5363b27 Compare March 25, 2024 10:50
@mbondyra mbondyra marked this pull request as ready for review March 25, 2024 14:50
@mbondyra mbondyra force-pushed the lens/chart_switch_into_one branch 2 times, most recently from c7ddfb3 to fbabd5e Compare March 25, 2024 20:38
Copy link

@gvnmagni gvnmagni left a comment

Choose a reason for hiding this comment

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

The PR does exactly what it says, works smoothly and I didn't encounter any issue while testing it.

I have a single minor comment that I will explain async since it doesn't affect in any way how the functionality works.

@mbondyra mbondyra force-pushed the lens/chart_switch_into_one branch from fbabd5e to d03aa41 Compare March 26, 2024 09:38
@elastic elastic deleted a comment from kibana-ci Mar 26, 2024
Copy link
Contributor

@dej611 dej611 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 found no issues.
I've some minor comments on code, but will approve now to unblock merge once fixed those.

Comment on lines 487 to 489
suggestions.find((s) => s.changeType === 'unchanged' || s.changeType === 'reduced') ||
suggestions.find((s) => s.keptLayerIds.some((id) => id === layerId)) ||
suggestions[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include the check in the existing loop?

Suggested change
suggestions.find((s) => s.changeType === 'unchanged' || s.changeType === 'reduced') ||
suggestions.find((s) => s.keptLayerIds.some((id) => id === layerId)) ||
suggestions[0]
suggestions.find((s) =>
s.changeType === 'unchanged' ||
s.changeType === 'reduced' ||
s.keptLayerIds.some((id) => id === layerId)
) || suggestions[0]

Copy link
Contributor Author

@mbondyra mbondyra Mar 26, 2024

Choose a reason for hiding this comment

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

actually no, because we want to give a bigger priority to unchanged or reduced types. And only if we cannot find one of these, we'll be searching through the rest (changeType: layers is what matters here)

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to add a comment about it then here to avoid in the future possible refactoring.

@@ -174,8 +137,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

await PageObjects.lens.configureDimension({
dimension: 'lns-layerPanel-1 > lnsXY_yDimensionPanel > lns-empty-dimension',
operation: 'median',
field: 'bytes',
operation: 'average',
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

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 was just lazy to add another translation to the localization tests. It tests the same thing so I think it's ok to change it. Let me know if you're ok with it.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
lens 563 568 +5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.4MB 1.4MB -1.3KB
Unknown metric groups

API count

id before after diff
lens 664 669 +5

History

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

@mbondyra mbondyra merged commit ef40d31 into elastic:main Mar 26, 2024
17 checks passed
@mbondyra mbondyra deleted the lens/chart_switch_into_one branch March 26, 2024 20:20
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Presence Of Dual Chart Selectors Is Confusing
6 participants