[SharedUX] Add feedback snippet to SharedUX storybook by angeles-mb · Pull Request #235197 · elastic/kibana

angeles-mb · 2025-09-16T12:27:30Z

Closes #234863

Summary

Added feedback snippet to our Storybook with custom controls to test props
Integrated some UI tweaks suggested by @ek-so Quick tweaks for the feedback snippet #234499
- A top divider for the feedback button
- Smaller buttons

Testing

Tested divider on scrollable submenus:

Tested divider on non-scrollable submenus:

Storybook looks:

elasticmachine · 2025-09-16T12:28:09Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

angeles-mb · 2025-09-16T12:29:44Z

src/platform/packages/shared/shared-ux/feedback_snippet/src/confetti/index.tsx

+
+export const Confetti = () => (
+  <Suspense fallback={null}>
+    <LazyConfetti />


Had to wrap Confetti component in Suspense to avoid this error which happened both in Storybook and when testing the component in other parts of Kibana Error: A component suspended while responding to synchronous input. This will cause the UI to be replaced with a loading indicator. To fix, updates that suspend should be wrapped with startTransition.

By doing this, FeedbackSnippet consumers don't have to worry about its lazy loading.

nit: you can also use dynamic

import { dynamic } from '@kbn/shared-ux-utility'; export const Confetti = dynamic(() => import('./confetti'));

Confetti also doesn't require a default export. You can remove the eslint disable and default export and use dynamic for that, too:

import { dynamic } from '@kbn/shared-ux-utility'; export const Confetti = dynamic(() => import('./confetti').then( mod => ({default: mod.Confetti}) );

I didn't know about that dynamic utility, thanks for sharing! Addressed!

Dosant

lgtm,
have a suggestion to consider

Dosant · 2025-09-16T14:49:53Z

...chrome/browser-internal/src/ui/project/sidenav_v2/navigation/navigation_feedback_snippet.tsx

      feedbackSnippetId={feedbackSnippetId}
      promptViewMessage={promptViewMessage}
      surveyUrl={feedbackSurveyUrl}
+      showFeedbackButtonTopDivider={true}


This feels very specific. Maybe this should just be a CSS override or addition instead.

<FeedbackSnippet css={`top-divider-css-goes-here`} feedbackButtonMessage={feedbackButtonMessage} feedbackSnippetId={feedbackSnippetId} promptViewMessage={promptViewMessage} surveyUrl={feedbackSurveyUrl}

Another option would be to encapsulate this change with some more expressive prop, like type or contained. For example, if we want to include a top divider when the FeedbackSnippet is contained within a panel, you can use "contained", or similar.

In other words, add a more expressive prop. From this, I don't know why I would or would not set this prop.

I agree it is too specific, I went with the css + className approach for the NavigationFeedbackSnippet to be able to use custom styling (only for the Button, not the Panel).

clintandrewhall

Great job! Please address these nits before committing.

clintandrewhall · 2025-09-16T15:33:01Z

src/platform/packages/shared/shared-ux/feedback_snippet/src/confetti/index.tsx

+
+export const Confetti = () => (
+  <Suspense fallback={null}>
+    <LazyConfetti />


nit: you can also use dynamic

import { dynamic } from '@kbn/shared-ux-utility'; export const Confetti = dynamic(() => import('./confetti'));

Confetti also doesn't require a default export. You can remove the eslint disable and default export and use dynamic for that, too:

import { dynamic } from '@kbn/shared-ux-utility'; export const Confetti = dynamic(() => import('./confetti').then( mod => ({default: mod.Confetti}) );

clintandrewhall · 2025-09-16T15:35:48Z

src/platform/packages/shared/shared-ux/feedback_snippet/src/feedback_button.tsx

      css={css`
-        margin: ${euiTheme.size.m};
-        padding: ${euiTheme.size.s};
+        ${showTopDivider && `border-top: 1px ${euiTheme.colors.borderBaseSubdued} solid;`}


nit: isn't 1px a border width on euiTheme, as well?

Suggested change

${showTopDivider && `border-top: 1px ${euiTheme.colors.borderBaseSubdued} solid;`}

${showTopDivider && `border-top: ${euiTheme.border.width.thin} ${euiTheme.colors.borderBaseSubdued} solid;`}

@eokoneyo Shouldn't this have thrown an eslint warning?

clintandrewhall · 2025-09-16T15:37:35Z

src/platform/packages/shared/shared-ux/feedback_snippet/src/feedback_panel.tsx

 } from '@elastic/eui';
 import { FormattedMessage } from '@kbn/i18n-react';
 import type { FeedbackView } from './feedback_snippet';
+import Confetti from './confetti/confetti';


nit: prefer a barrel import. If you're importing the non-lazy component, export it from the barrel as ConfettiComponent.

Oh yes, thanks for noticing!

clintandrewhall · 2025-09-16T15:40:14Z

...chrome/browser-internal/src/ui/project/sidenav_v2/navigation/navigation_feedback_snippet.tsx

      feedbackSnippetId={feedbackSnippetId}
      promptViewMessage={promptViewMessage}
      surveyUrl={feedbackSurveyUrl}
+      showFeedbackButtonTopDivider={true}


Another option would be to encapsulate this change with some more expressive prop, like type or contained. For example, if we want to include a top divider when the FeedbackSnippet is contained within a panel, you can use "contained", or similar.

In other words, add a more expressive prop. From this, I don't know why I would or would not set this prop.

clintandrewhall · 2025-09-16T15:40:30Z

src/platform/packages/shared/shared-ux/feedback_snippet/src/confetti/index.tsx

+
+const LazyConfetti = lazy(() => import('./confetti'));
+
+export const Confetti = () => (


nit: exports need documentation.

elasticmachine · 2025-09-17T12:23:12Z

💛 Build succeeded, but was flaky

Failed CI Steps

Jest Integration Tests #6

Test Failures

[job] [logs] Jest Integration Tests #6 / unrecognized task types should be no workload aggregator errors when there are removed task types

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`core`	562	563	+1

Async chunks

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

id	before	after	diff
`core`	131.2KB	131.7KB	+485.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	511.3KB	511.3KB	+2.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`@kbn/shared-ux-feedback-snippet`	1	0	-1

Total ESLint disabled count

id	before	after	diff
`@kbn/shared-ux-feedback-snippet`	1	0	-1

History

💚 Build #339815 succeeded 599446a

cc @angeles-mb

@ek-so

Closes elastic#234863 ## Summary - Added feedback snippet to our Storybook with custom controls to test props - Integrated some UI tweaks suggested by @ek-so elastic#234499 - A top divider for the feedback button - Smaller buttons ## Testing Tested divider on scrollable submenus: <img width="743" height="1051" alt="Screenshot 2025-09-16 at 12 05 47" src="https://github.com/user-attachments/assets/75589637-9adc-4bfb-99a1-ccccf50b00e9" /> Tested divider on non-scrollable submenus: <img width="743" height="1043" alt="Screenshot 2025-09-16 at 13 06 05" src="https://github.com/user-attachments/assets/69ba7b47-b3e0-4624-aa1a-0d1ea7f32e6b" /> Storybook looks: <img width="895" height="1019" alt="Screenshot 2025-09-16 at 12 07 13" src="https://github.com/user-attachments/assets/dba37ae4-9055-4b6b-8494-870c35f81890" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

@ek-so

Closes #234863 ## Summary - Added feedback snippet to our Storybook with custom controls to test props - Integrated some UI tweaks suggested by @ek-so #234499 - A top divider for the feedback button - Smaller buttons ## Testing Tested divider on scrollable submenus: <img width="743" height="1051" alt="Screenshot 2025-09-16 at 12 05 47" src="https://github.com/user-attachments/assets/75589637-9adc-4bfb-99a1-ccccf50b00e9" /> Tested divider on non-scrollable submenus: <img width="743" height="1043" alt="Screenshot 2025-09-16 at 13 06 05" src="https://github.com/user-attachments/assets/69ba7b47-b3e0-4624-aa1a-0d1ea7f32e6b" /> Storybook looks: <img width="895" height="1019" alt="Screenshot 2025-09-16 at 12 07 13" src="https://github.com/user-attachments/assets/dba37ae4-9055-4b6b-8494-870c35f81890" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

weronikaolejniczak · 2025-10-15T12:07:40Z

...chrome/browser-internal/src/ui/project/sidenav_v2/navigation/navigation_feedback_snippet.tsx


  return (
    <FeedbackSnippet
+      css={css`


@angeles-mb this border should likely be on side nav side, and removed in high contrast mode.

Adding this on the side nav side will also add it for the feedback panel. Is this what we want?

I think the panel looks better without it but let me know your thoughts @ek-so @weronikaolejniczak

@angeles-mb IMO the panel looks better without it as well.

Because we want to expose this panel slot for solution teams to possibly put rich content there, would we need the separation? That is also something to consider.

@angeles-mb Wait, I'm confused 🤔 Looking at the code, FeedbackSnippet encompasses the whole feedback including the "Navigation feedback" redirection button. So how is adding the border-top different here than adding it to sidePanelFooter slot?

@weronikaolejniczak That's because the className styles are intentionally not being propagated to the FeedbackPanel but only to the FeedbackButton.

My initial approach was to have a specific boolean prop to decide whether to show the divider or not for the button. See here. I know the current approach is not transparent for FeedbackSnippet consumers.

In any case, I'm ok with moving it to the SideNav if that is what we want for both flavors and for other potential content on that footer.

@angeles-mb hmm I don't like either option, to be honest - a boolean flag or CSS override. It's not immediately clear that FeedbackPanel doesn't inherit these styles. IMHO it's side nav container that should say "I want there to be a border between the menu and footer slot" regardless of the footer slot content. It feels more expected.

That being said, the result would be ☝🏻 and I agree with you, probably @ek-so does as well, that it doesn't look good. So let's leave it as is for now 😄 Thanks for explaining!

Yes, we have a lot of borders everywhere... cleaner without it here 🙈

weronikaolejniczak · 2025-10-15T12:09:08Z

src/platform/packages/shared/shared-ux/feedback_snippet/src/feedback_button.tsx

+        onClick={handleOpenSurvey}
+        css={css`
+          width: 100%;
+          padding: ${euiTheme.size.s};


@ek-so is this expected to be overriding the padding of the EuiButtonEmpty? 🤔 The way I see it the consumer should just be able to say size="s" and everything adjust accordingly. Otherwise, we introduce inconsistencies.

Tbh I don't understand what does this prop do here (I mean, literally, does it have any visual effect)? To me it looks like it doesn't 🤔

Thanks for reviewing! This padding was coming from your original PR @ek-so but it's true that it doesn't add much as you mentioned: button and container sizes are the same with (left) and without (right) the padding:

We can easily remove the padding.

@angeles-mb perfect! Thanks for checking, Angeles.

Thank you both! It seems, I forgot to remove 🙈

@ek-so

Closes elastic#234863 ## Summary - Added feedback snippet to our Storybook with custom controls to test props - Integrated some UI tweaks suggested by @ek-so elastic#234499 - A top divider for the feedback button - Smaller buttons ## Testing Tested divider on scrollable submenus: <img width="743" height="1051" alt="Screenshot 2025-09-16 at 12 05 47" src="https://github.com/user-attachments/assets/75589637-9adc-4bfb-99a1-ccccf50b00e9" /> Tested divider on non-scrollable submenus: <img width="743" height="1043" alt="Screenshot 2025-09-16 at 13 06 05" src="https://github.com/user-attachments/assets/69ba7b47-b3e0-4624-aa1a-0d1ea7f32e6b" /> Storybook looks: <img width="895" height="1019" alt="Screenshot 2025-09-16 at 12 07 13" src="https://github.com/user-attachments/assets/dba37ae4-9055-4b6b-8494-870c35f81890" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

angeles-mb self-assigned this Sep 16, 2025

angeles-mb requested a review from a team as a code owner September 16, 2025 12:27

angeles-mb added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// labels Sep 16, 2025

angeles-mb commented Sep 16, 2025

View reviewed changes

Dosant approved these changes Sep 16, 2025

View reviewed changes

clintandrewhall reviewed Sep 16, 2025

View reviewed changes

angeles-mb added 2 commits September 17, 2025 09:55

[SharedUX] Add feedback snippet to SharedUX storybook

db010a2

Use dynamic utility for Suspense logic and remove top border prop

82bf2d9

angeles-mb force-pushed the 234863-add-feedback-snippet-to-shared-ux-storybook branch from 599446a to 82bf2d9 Compare September 17, 2025 09:56

[CI] Auto-commit changed files from 'node scripts/notice'

847857d

angeles-mb merged commit 95b0206 into elastic:main Sep 18, 2025
13 checks passed

kibanamachine added the v9.2.0 label Sep 18, 2025

weronikaolejniczak reviewed Oct 15, 2025

View reviewed changes

	${showTopDivider && `border-top: 1px ${euiTheme.colors.borderBaseSubdued} solid;`}
	${showTopDivider && `border-top: ${euiTheme.border.width.thin} ${euiTheme.colors.borderBaseSubdued} solid;`}


		const LazyConfetti = lazy(() => import('./confetti'));

		export const Confetti = () => (

Conversation

angeles-mb commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

elasticmachine commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintandrewhall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 17, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angeles-mb Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

angeles-mb commented Sep 16, 2025 •

edited

Loading

angeles-mb Oct 16, 2025 •

edited

Loading