[TIP] Fix barchart legend rendering issue #142268

PhilippeOberti · 2022-09-29T20:00:51Z

Summary

Our Indicators barchart legend component was cutting off names too aggressively.

Also as seen in the screenshot above, the 3-dot button used to display the context menu for actions wasn't properly aligned with the rest of the legend.

This PR:

set minimum width to 200px (this is not ideal but the elastic-charts' legend component has a built-in logic that will make sure the legend can't be wider than 30% of the total width. I interpreted the legendSize prop as a min-size)
vertically align context menu button (this seems to be a problem with the elastic-charts' implementation itself (looking at their Storybook seems to have the same problem).

Render after fix:

Fixes https://github.com/elastic/security-team/issues/5117

- set minimum width - vertically align context menu button

lgestc · 2022-09-30T07:47:14Z

.../indicators/components/indicator_barchart_legend_action/indicator_barchart_legend_action.tsx

+              iconType="boxesHorizontal"
+              iconSize="s"
+              size="xs"
+              onClick={() => setPopover(!isPopoverOpen)}


Hey, in this particular situation, the recommended approach is to use https://reactjs.org/docs/hooks-reference.html#functional-updates - check it out :)

Also, maybe we could extract this handler with useCallback?

thanks I wasn't aware of the functional updates, it's cool as it prevent rerendering if the value is identical!

It seems to me that the useCallback here is overkill. We're not really rendering a complex component, just a EuiContextPanel with 3 icons and 3 texts... No?

lgestc · 2022-09-30T07:48:12Z

.../indicators/components/indicator_barchart_legend_action/indicator_barchart_legend_action.tsx

+          </EuiToolTip>
+        }
+        isOpen={isPopoverOpen}
+        closePopover={() => setPopover(false)}


useCallback would work better here as well

lgestc

LGTM, with some minor notes. Apply the changes or skip them, I think the performance needs a deeper dive anyway around the entire plugin, but these useCallbacks are a low hanging fruit that does not hurt I guess 👯

maxcold

overall good improvement, couple of questions to the chart library itself on which we can kick off a discussion with the team owning it

maxcold · 2022-09-30T09:05:07Z

...ntelligence/public/modules/indicators/components/indicators_barchart/indicators_barchart.tsx

@@ -52,6 +53,7 @@ export const IndicatorsBarChart: VFC<IndicatorsBarChartProps> = ({
          showLegend
          showLegendExtra


Took a look at the charts documentation. It's quite weird that the legend is controlled only through the Settings component. My suggestion is to remove the showLegendExtra from the settings because values in the legend in my opinion don't bring any value, only confusion. Tbh the current behaviour of the legend looks to my more like a UX bug than a feature, especially when combined with a tooltip on hower on each bar. Does it makes sense to consult with the team owning the chart library and ask how would they suggest to implement such a feature - showing the accumulated values in the legend

found this issue that is related to the problem elastic/elastic-charts#561

yeah good point on removing the showLegendExtra, I really don't see what value it provide at the moment as we have the tooltip that's way more convenient.

I'll reach out to the charts team and ask them about the accumulated values in legend!

maxcold · 2022-09-30T09:14:54Z

...intelligence/public/modules/indicators/components/indicator_barchart_legend_action/styles.ts

+
+export const useStyles = () => {
+  const contextMenu: CSSObject = {
+    'margin-top': '-2px',


this is quite weird that we need to do this hacks to align the components that are built in. Does it makes sense to also reach out to the team to check if it is a bug they are aware of.

I agree I hated overriding some default values, but hated even more the -2px.

After investigating more by playing with the elastic-charts repo, the problem is actually with the use of the EuiTooltip. Removing it fixes the alignment issue. I posted on their channel , let's see it I'm doing something wrong.
For now I decided to remove the EuiToolTip, it will take 30 seconds to add it back once we figure out how to do it properly...

well actually with the help of the charts team I think we have a better hack for now. We're using style={{ height: '100%' }} on the EuiButtonIcon. It's either that or we don't use EuButtonIcon and have to style everything ourselves, like they do in the elastic-charts Storybook here.

The charts team isn't really testing their components with Eui components so it's not the first time they're seeing those weird UI issues...

created a bug ticket on the elastic-charts repo for traceability

# Conflicts: # x-pack/plugins/threat_intelligence/public/modules/indicators/components/barchart/legend_action/legend_action.tsx # x-pack/plugins/threat_intelligence/public/modules/indicators/components/barchart/legend_action/styles.ts

…ue shown in legend

kibana-ci · 2022-09-30T21:09:08Z

💚 Build Succeeded

Buildkite Build
Commit: 2e1ffcc

Metrics [docs]

Async chunks

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

id	before	after	diff
`threatIntelligence`	121.1KB	121.1KB	+23.0B

History

💚 Build #76988 succeeded c052347

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

* [TIP] Fix barchart legend rendering issue - set minimum width - vertically align context menu button - remove value displayed in legend

[TIP] Fix barchart legend rendering issue

c052347

- set minimum width - vertically align context menu button

PhilippeOberti requested a review from a team as a code owner September 29, 2022 20:00

PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes 8.5 candidate Team: Protections Experience v8.6.0 labels Sep 29, 2022

lgestc reviewed Sep 30, 2022

View reviewed changes

lgestc approved these changes Sep 30, 2022

View reviewed changes

maxcold approved these changes Sep 30, 2022

View reviewed changes

PhilippeOberti added 2 commits September 30, 2022 10:40

Merge branch 'main' into barchart-legend-width

b2a67ab

# Conflicts: # x-pack/plugins/threat_intelligence/public/modules/indicators/components/barchart/legend_action/legend_action.tsx # x-pack/plugins/threat_intelligence/public/modules/indicators/components/barchart/legend_action/styles.ts

[TIP] Add styles to EuiButton icon to fix misalignment and remove val…

2e1ffcc

…ue shown in legend

PhilippeOberti force-pushed the barchart-legend-width branch from b9dd471 to 2e1ffcc Compare September 30, 2022 19:41

PhilippeOberti merged commit 1b09c82 into elastic:main Sep 30, 2022

kibanamachine added the backport:skip This commit does not require backporting label Sep 30, 2022

PhilippeOberti deleted the barchart-legend-width branch September 30, 2022 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIP] Fix barchart legend rendering issue #142268

[TIP] Fix barchart legend rendering issue #142268

PhilippeOberti commented Sep 29, 2022

lgestc Sep 30, 2022

lgestc Sep 30, 2022

PhilippeOberti Sep 30, 2022

lgestc Sep 30, 2022

lgestc left a comment

maxcold left a comment

maxcold Sep 30, 2022

maxcold Sep 30, 2022

PhilippeOberti Sep 30, 2022

maxcold Sep 30, 2022

PhilippeOberti Sep 30, 2022

PhilippeOberti Sep 30, 2022

PhilippeOberti Sep 30, 2022

kibana-ci commented Sep 30, 2022

		@@ -52,6 +53,7 @@ export const IndicatorsBarChart: VFC<IndicatorsBarChartProps> = ({
		showLegend
		showLegendExtra

[TIP] Fix barchart legend rendering issue #142268

[TIP] Fix barchart legend rendering issue #142268

Conversation

PhilippeOberti commented Sep 29, 2022

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgestc left a comment

Choose a reason for hiding this comment

maxcold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Sep 30, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History