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

[Storybook] Add playground stories for remaining C components #7467

Merged
merged 7 commits into from
Jan 18, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jan 17, 2024

Summary

part 2 of #7460

This PR is slightly more complex than the previous one, as it:

  1. Modifies/enhances components with existing storybooks
  2. Fixes several types
  3. Removes a subcomponent top-level public export that is not being used anywhere in Elastic, and probably was a mistaken export since it doesn't export any corresponding types

Note: Any TODOs in this PR were left as general tech debt / are not meant to be completed in this PR.

QA

General checklist

N/A, docs only

- add missing defaults
- add missing Picked props :| (storybook why?? need to figure this out)

- add support for `onCreateOption` UX
- make story `onChange` and `onCreateOption` show up in actions pane
- use generic for nav or div DOM element type instead of casting `as`

- add missing control defaults

- register onClose action in storybook tab
- not used by any other Elastic products, and unlikely to be used standalone
+ fix missing `children` in exported EuiContextMenuItemProps
- there's a lot of TODOs at the bottom here because this is honestly kind of a weird component that feels very legacy
@cee-chen cee-chen added documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog labels Jan 17, 2024
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen cee-chen marked this pull request as ready for review January 17, 2024 19:49
@cee-chen cee-chen requested a review from a team as a code owner January 17, 2024 19:49
@JasonStoltz JasonStoltz self-assigned this Jan 17, 2024
Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

Both of these are probably suggestions rather than blockers.


import { EuiButton } from '../button';
import { EuiCollapsibleNav, EuiCollapsibleNavProps } from './collapsible_nav';

const meta: Meta<EuiCollapsibleNavProps> = {
title: 'EuiCollapsibleNav',
component: EuiCollapsibleNav,
argTypes: {
...disableStorybookControls(['button']),
Copy link
Member

Choose a reason for hiding this comment

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

Would this be better as text?

diff --git a/src/components/collapsible_nav/collapsible_nav.stories.tsx b/src/components/collapsible_nav/collapsible_nav.stories.tsx
index 777f63732..1250f1140 100644
--- a/src/components/collapsible_nav/collapsible_nav.stories.tsx
+++ b/src/components/collapsible_nav/collapsible_nav.stories.tsx
@@ -18,9 +18,10 @@ const meta: Meta<EuiCollapsibleNavProps> = {
   title: 'EuiCollapsibleNav',
   component: EuiCollapsibleNav,
   argTypes: {
-    ...disableStorybookControls(['button']),
+    ...disableStorybookControls([]),
     as: { options: ['nav', 'div'], control: 'radio' },
     maxWidth: { control: 'number' }, // TODO: also accepts bool | string
+    button: { control: 'tex
<img width="1561" alt="Screenshot 2024-01-17 at 4 49 46 PM" src="https://github.com/elastic/eui/assets/1427475/d470b54f-b1b8-4343-980b-d9be6eae7dd9">
t' },
   },
   args: {
     // Component defaults
@@ -38,6 +39,7 @@ const meta: Meta<EuiCollapsibleNavProps> = {
     includeFixedHeadersInFocusTrap: true,
     outsideClickCloses: true,
     ownFocus: true,
+    button: 'Toggle nav',
   },
   // TODO: Improve props inherited from EuiFlyout, ideally through
   // a DRY import from `flyout.stories.tsx` once that's created
@@ -48,13 +50,14 @@ type Story = StoryObj<EuiCollapsibleNavProps>;
 
 const StatefulCollapsibleNav = (props: Partial<EuiCollapsibleNavProps>) => {
   const [isOpen, setIsOpen] = useState(props.isOpen);
+  const button = props.button;
   return (
     <EuiCollapsibleNav
       {...props}
       isOpen={isOpen}
       button={
         <EuiButton onClick={() => setIsOpen((isOpen) => !isOpen)}>
-          Toggle nav
+          {button}
         </EuiButton>
       }
       onClose={(...args) => {
Screenshot 2024-01-17 at 4 49 46 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, I don't think so. IMO, it's confusing/not a good example of what a consumer would actually pass to the button prop, which would be something like: button={<EuiButton onClick={toggleCollapsibleNav} />}. The controls should try to provide code examples for consumers if possible, and setting this to a string would indicate a button automagically renders if they pass button="Open me", which is not the case.

event: 'added a comment',
timestamp: 'on Jan 1, 2020',
children: (
<EuiText size="s">
Copy link
Member

Choose a reason for hiding this comment

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

Nesting any EUI elements in this object causes the following error in the playground whenever you tweak this object:

Screenshot 2024-01-17 at 5 03 00 PM

No idea what the problem is -- I'm guessing something to do with Emotion and rerendering, based on the error message and the fact that it happens on re-render.

We could simply remove the complex nested items for now so at least the playground works:

diff --git a/src/components/comment_list/comment_list.stories.tsx b/src/components/comment_list/comment_list.stories.tsx
index 8eab65bf7..08e379474 100644
--- a/src/components/comment_list/comment_list.stories.tsx
+++ b/src/components/comment_list/comment_list.stories.tsx
@@ -9,11 +9,6 @@
 import React from 'react';
 import type { Meta, StoryObj } from '@storybook/react';
 
-import { EuiText } from '../text';
-import { EuiButtonIcon } from '../button';
-import { EuiBadge } from '../badge';
-import { EuiFlexGroup, EuiFlexItem } from '../flex';
-
 import { EuiCommentList, EuiCommentListProps } from './comment_list';
 
 const meta: Meta<EuiCommentListProps> = {
@@ -28,15 +23,6 @@ const meta: Meta<EuiCommentListProps> = {
 export default meta;
 type Story = StoryObj<EuiCommentListProps>;
 
-const copyAction = (
-  <EuiButtonIcon
-    title="Custom action"
-    aria-label="Custom action"
-    color="text"
-    iconType="copy"
-  />
-);
-
 export const Playground: Story = {
   args: {
     comments: [
@@ -46,20 +32,15 @@ export const Playground: Story = {
         event: 'added a comment',
         timestamp: 'on Jan 1, 2020',
         children: (
-          <EuiText size="s">
-            <p>
-              Far out in the uncharted backwaters of the unfashionable end of
-              the western spiral arm of the Galaxy lies a small unregarded
-              yellow sun.
-            </p>
-          </EuiText>
+          <p>
+            Far out in the uncharted backwaters of the unfashionable end of the
+            western spiral arm of the Galaxy lies a small unregarded yellow sun.
+          </p>
         ),
-        actions: copyAction,
       },
       {
         username: 'juanab',
         timelineAvatarAriaLabel: 'Juana Barros',
-        actions: copyAction,
         event: 'pushed incident X0Z235',
         timestamp: 'on Jan 3, 2020',
       },
@@ -74,26 +55,7 @@ export const Playground: Story = {
       {
         username: 'pedror',
         timelineAvatarAriaLabel: 'Pedro Rodriguez',
-        actions: copyAction,
-        event: (
-          <EuiFlexGroup
-            responsive={false}
-            alignItems="center"
-            gutterSize="xs"
-            wrap
-          >
-            <EuiFlexItem grow={false}>added tags</EuiFlexItem>
-            <EuiFlexItem grow={false}>
-              <EuiBadge>case</EuiBadge>
-            </EuiFlexItem>
-            <EuiFlexItem grow={false}>
-              <EuiBadge>phising</EuiBadge>
-            </EuiFlexItem>
-            <EuiFlexItem grow={false}>
-              <EuiBadge>security</EuiBadge>
-            </EuiFlexItem>
-          </EuiFlexGroup>
-        ),
+        event: "added the 'case' tag",
         timestamp: 'on Jan 11, 2020',
         eventIcon: 'tag',
         eventIconAriaLabel: 'tag',
@@ -103,7 +65,6 @@ export const Playground: Story = {
         timelineAvatarAriaLabel: 'Assistant',
         timestamp: 'on Jan 14, 2020, 1:39:04 PM',
         children: <p>An error ocurred sending your message.</p>,
-        actions: copyAction,
         eventColor: 'danger',
       },
     ],
Screenshot 2024-01-17 at 5 08 38 PM

Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

Sorry, some additions to my last comments. Should have grouped these all together, sorry for the noise.

@cee-chen
Copy link
Member Author

@JasonStoltz A lot of your comments are critiques of Storybook's controls UI/UX, which I 100% agree with. While it's still a step up from our current docs, mostly in terms of responsiveness and discoverability, it's not perfect and it can't be without us taking the time to sit down and write our own custom Storybook addon to modify their controls. This is something I'd definitely like to do, but then we're kind of up against the question of: what's the goal of EUI+ and our timeline(s) on it? We can stop and make it significantly better along the way, but that will almost certainly mean blowing past the current timeline you have set. If we're simply trying to get the current iteration of Docusaurus vaguely up to where our current docs are now, then these aren't friction points we can currently address.

@JasonStoltz
Copy link
Member

OK, let's just focus on getting these all migrated to storybook and we can circle back to improve these in the future if necessary.

@cee-chen
Copy link
Member Author

cee-chen commented Jan 18, 2024

if necessary

It's definitely necessary, just IMO! But probably a fairly involved endeavor to get the DX perfect. These comments have been super helpful in exploring better alternatives!

@cee-chen cee-chen merged commit 434c4d1 into elastic:main Jan 18, 2024
8 checks passed
@cee-chen cee-chen deleted the storybook/c-2 branch January 18, 2024 18:03
cee-chen added a commit to elastic/kibana that referenced this pull request Jan 30, 2024
`v92.1.1`⏩`v92.2.1`

---

## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1)

**Bug fixes**

- Removed unintentional i18n tokens in prior release that should not
have been exported

## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0)

- Updated `EuiFlyoutResizable` with new optional `onResize` callback
([#7464](elastic/eui#7464))

**Bug fixes**

- Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could
become a stale closure when renders occured between resize start and
end, resulting in an outdated version of a consumer's `onResizeEnd`
callback being called
([#7468](elastic/eui#7468))
- Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear
button click ([#7473](elastic/eui#7473))
- Fixed `EuiContextMenu`'s panel titles & items to not show underlines
on hover for non-interactive elements
([#7474](elastic/eui#7474))

**Deprecations**

- Remove unused public `EuiHue` and `EuiSaturation` subcomponent
exports. Use the parent `EuiColorPicker` component instead
([#7460](elastic/eui#7460))
- Remove unused public `EuiCommentTimeline` subcomponent export. Use the
parent `EuiComment` or `EuiCommentList` components instead.
([#7467](elastic/eui#7467))
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
`v92.1.1`⏩`v92.2.1`

---

## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1)

**Bug fixes**

- Removed unintentional i18n tokens in prior release that should not
have been exported

## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0)

- Updated `EuiFlyoutResizable` with new optional `onResize` callback
([elastic#7464](elastic/eui#7464))

**Bug fixes**

- Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could
become a stale closure when renders occured between resize start and
end, resulting in an outdated version of a consumer's `onResizeEnd`
callback being called
([elastic#7468](elastic/eui#7468))
- Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear
button click ([elastic#7473](elastic/eui#7473))
- Fixed `EuiContextMenu`'s panel titles & items to not show underlines
on hover for non-interactive elements
([elastic#7474](elastic/eui#7474))

**Deprecations**

- Remove unused public `EuiHue` and `EuiSaturation` subcomponent
exports. Use the parent `EuiColorPicker` component instead
([elastic#7460](elastic/eui#7460))
- Remove unused public `EuiCommentTimeline` subcomponent export. Use the
parent `EuiComment` or `EuiCommentList` components instead.
([elastic#7467](elastic/eui#7467))
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
`v92.1.1`⏩`v92.2.1`

---

## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1)

**Bug fixes**

- Removed unintentional i18n tokens in prior release that should not
have been exported

## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0)

- Updated `EuiFlyoutResizable` with new optional `onResize` callback
([elastic#7464](elastic/eui#7464))

**Bug fixes**

- Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could
become a stale closure when renders occured between resize start and
end, resulting in an outdated version of a consumer's `onResizeEnd`
callback being called
([elastic#7468](elastic/eui#7468))
- Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear
button click ([elastic#7473](elastic/eui#7473))
- Fixed `EuiContextMenu`'s panel titles & items to not show underlines
on hover for non-interactive elements
([elastic#7474](elastic/eui#7474))

**Deprecations**

- Remove unused public `EuiHue` and `EuiSaturation` subcomponent
exports. Use the parent `EuiColorPicker` component instead
([elastic#7460](elastic/eui#7460))
- Remove unused public `EuiCommentTimeline` subcomponent export. Use the
parent `EuiComment` or `EuiCommentList` components instead.
([elastic#7467](elastic/eui#7467))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants