-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[SharedUX] Add feedback snippet to SharedUX storybook #235197
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import { dynamic } from '@kbn/shared-ux-utility'; | ||
|
|
||
| /** | ||
| * A lazy-loaded component that renders a one-shot confetti animation. | ||
| */ | ||
|
|
||
| export const Confetti = dynamic(() => | ||
| import('./confetti.component').then((mod) => ({ | ||
| default: mod.ConfettiComponent, | ||
| })) | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,10 +10,11 @@ | |
| import React from 'react'; | ||
| import { css } from '@emotion/react'; | ||
|
|
||
| import type { CommonProps } from '@elastic/eui'; | ||
| import { EuiButtonEmpty, useEuiTheme } from '@elastic/eui'; | ||
| import { i18n } from '@kbn/i18n'; | ||
|
|
||
| interface FeedbackButtonProps { | ||
| interface FeedbackButtonProps extends CommonProps { | ||
| feedbackButtonMessage: React.ReactNode; | ||
| feedbackSnippetId: string; | ||
| handleOpenSurvey: () => void; | ||
|
|
@@ -35,24 +36,33 @@ export const FeedbackButton = ({ | |
| feedbackButtonMessage, | ||
| feedbackSnippetId, | ||
| handleOpenSurvey, | ||
| className, | ||
| }: FeedbackButtonProps) => { | ||
| const { euiTheme } = useEuiTheme(); | ||
|
|
||
| return ( | ||
| <EuiButtonEmpty | ||
| data-test-subj="feedbackSnippetButton" | ||
| onClick={handleOpenSurvey} | ||
| <div | ||
| className={className} | ||
| css={css` | ||
| margin: ${euiTheme.size.m}; | ||
| padding: ${euiTheme.size.s}; | ||
| padding: ${euiTheme.size.m}; | ||
| `} | ||
| color="text" | ||
| iconType="popout" | ||
| iconSide="right" | ||
| id={`${feedbackSnippetId}ButtonSurveyLink`} | ||
| aria-label={feedbackButtonAriaLabel} | ||
| > | ||
| {feedbackButtonMessage} | ||
| </EuiButtonEmpty> | ||
| <EuiButtonEmpty | ||
| data-test-subj="feedbackSnippetButton" | ||
| onClick={handleOpenSurvey} | ||
| css={css` | ||
| width: 100%; | ||
| padding: ${euiTheme.size.s}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ek-so is this expected to be overriding the padding of the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @angeles-mb perfect! Thanks for checking, Angeles.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you both! It seems, I forgot to remove 🙈 |
||
| `} | ||
| color="text" | ||
| iconType="popout" | ||
| iconSide="right" | ||
| size="s" | ||
| id={`${feedbackSnippetId}ButtonSurveyLink`} | ||
| aria-label={feedbackButtonAriaLabel} | ||
| > | ||
| {feedbackButtonMessage} | ||
| </EuiButtonEmpty> | ||
| </div> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import React from 'react'; | ||
| import type { Meta, StoryObj } from '@storybook/react'; | ||
| import { FeedbackSnippet } from './feedback_snippet'; | ||
|
|
||
| const meta: Meta<typeof FeedbackSnippet> = { | ||
| title: 'Shared UX/FeedbackSnippet', | ||
| component: FeedbackSnippet, | ||
| args: { | ||
| feedbackButtonMessage: 'Got feedback?', | ||
| promptViewMessage: 'Was this page helpful? Tell us about it!', | ||
| surveyUrl: 'https://www.elastic.co', | ||
| feedbackSnippetId: 'storybook-feedback-snippet-default', | ||
| }, | ||
| argTypes: { | ||
| feedbackButtonMessage: { control: 'text' }, | ||
| promptViewMessage: { control: 'text' }, | ||
| surveyUrl: { control: 'text' }, | ||
| feedbackSnippetId: { control: 'text' }, | ||
| }, | ||
| }; | ||
|
|
||
| export default meta; | ||
| type Story = StoryObj<typeof FeedbackSnippet>; | ||
|
|
||
| export const Default: Story = { | ||
| render: (args) => { | ||
| localStorage.removeItem(args.feedbackSnippetId); | ||
| return ( | ||
| <div | ||
| css={{ | ||
| width: '250px', | ||
| }} | ||
| > | ||
| <FeedbackSnippet {...args} /> | ||
| </div> | ||
| ); | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,5 +19,6 @@ | |
| "kbn_references": [ | ||
| "@kbn/i18n", | ||
| "@kbn/i18n-react", | ||
| "@kbn/shared-ux-utility", | ||
| ] | ||
| } | ||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angeles-mb this border should likely be on side nav side, and removed in high contrast mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angeles-mb Wait, I'm confused 🤔 Looking at the code,
FeedbackSnippetencompasses the whole feedback including the "Navigation feedback" redirection button. So how is adding theborder-topdifferent here than adding it tosidePanelFooterslot?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weronikaolejniczak That's because the
classNamestyles are intentionally not being propagated to theFeedbackPanelbut only to theFeedbackButton.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
FeedbackSnippetconsumers.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angeles-mb hmm I don't like either option, to be honest - a boolean flag or CSS override. It's not immediately clear that
FeedbackPaneldoesn'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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have a lot of borders everywhere... cleaner without it here 🙈