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

docs: add EuiComboBox examples #7470

Closed

Conversation

tkajtoch
Copy link
Member

Summary

This PR adds EuiComboBox docs examples to storybook and embeds them in the new docusaurus docs.

  • Type EuiComboBox.defaultProps to help storybook resolve component arg types
  • Add StoryEmbed component to docusaurus that renders storybook previews inside iframes
    • The logic of this component is not final and will be replaced with the proper implementation later this month including automatic container resizing, code snippet rendering and accessibility features
  • Add EuiComboBox examples to its new docs page

QA

  • Go to the EuiComboBox docs page and confirm the examples render correctly

@tkajtoch tkajtoch requested a review from a team as a code owner January 22, 2024 15:10
@tkajtoch
Copy link
Member Author

You may notice that the story embeds are slow to interact with. This is because all of the iframes are loaded from the same origin, and storybook sends messages to itself times the number of loaded iframes, which causes a huge performance hit for event listeners. The simplest fix is to enable sandbox="allow-scripts" on the iframe elements that limits that inter-connectivity, and I'll change that when working on improving the StoryEmbed component.

@cee-chen
Copy link
Member

cee-chen commented Jan 22, 2024

@tkajtoch I'm not seeing any way to get to the EuiComboBox docs anymore via the nav:

This is on staging https://eui.elastic.co/pr_7470/new-docs/docs/components/navigation/combo_box/testing/

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@tkajtoch
Copy link
Member Author

@cee-chen Looks like there were two combo_box directories after rebasing and the autogenerated sidebar got a little crazy. I fixed it in 646f181. Thanks for catching that!

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

This is coming together nicely @tkajtoch. I have two suggestions for accessibility. LMK if you have questions or want to discuss further.

}, []);
return (
<EuiComboBox
singleSelection={
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to add an accessible label to the comboboxes so they pass axe scans. I think aria-label makes the most sense here where we've mentioned options in the advisory panel above.

@@ -20,35 +20,49 @@ or passing a text node ID to the `aria-labelledby` prop.

:::

<StoryEmbed storyId="components-euicombobox--default" view="docs" height={230} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to add an accessible label to these rendered <iframe> components. Since you're already spreading props with { ...rest }, you should be able to pass an aria-label here with a unique string for each one.

This will help us prevent axe errors on full page scans.

Copy link
Member

Choose a reason for hiding this comment

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

Just curious - is there a way to pass Docusaurus's light/dark mode to the underlying Storybook embeds?

Comment on lines +7 to +14
let storybookBaseUrl = process.env.DOCS_STORYBOOK_BASE_URL;
if (!storybookBaseUrl) {
if (process.env.NODE_ENV !== 'production') {
storybookBaseUrl = 'http://localhost:6006';
} else {
storybookBaseUrl = 'https://eui.elastic.co/storybook';
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Random thought - instead of setting a new DOCS_STORYBOOK_BASE_URL env, could we not simply determine it based on the existing DOCS_BASE_URL env? e.g.

if (baseUrl.includes('localhost') {
  storybookBaseUrl = 'http://localhost:6006';
} else if (baseUrl.includes('/pr_')) {
  storybookBaseUrl = baseUrl.replace('/new-docs/', '') + '/storybook';
} else {
  storybookBaseUrl = 'https://eui.elastic.co/storybook';
}

@@ -27,6 +36,10 @@ const config: Config = {
locales: ['en'],
},

customFields: {
storybookBaseUrl: storybookBaseUrl,
Copy link
Member

Choose a reason for hiding this comment

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

me being annoying and nitty

Suggested change
storybookBaseUrl: storybookBaseUrl,
storybookBaseUrl,

Comment on lines -34 to -37
// Storybook is skipping the Pick<> props from EuiComboBoxList for some annoying reason
onCreateOption: { control: 'boolean' }, // Set to a true/false for ease of testing
customOptionText: { control: 'text' },
renderOption: { control: 'function' },
Copy link
Member

Choose a reason for hiding this comment

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

Why did this get removed?

},
args: {
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above - can we restore these args and replace the Default story with Playground? And I'd love for the playground to render with the control pane showing

@@ -206,7 +206,7 @@ export class EuiComboBox<T> extends Component<
_EuiComboBoxProps<T>,
EuiComboBoxState<T>
> {
static defaultProps = {
static defaultProps: Partial<_EuiComboBoxProps<object>> = {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, why was this change necessary?

Copy link
Member

Choose a reason for hiding this comment

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

I know this may be out of scope for this PR, but I really think it's worth considering streamlining our examples/documentation instead of adding stories we don't necessarily need. For example, I definitely want to see the isDisabled section be removed in favor of just the props/controls table. Honestly there's just so much organization that could be improved while we're here and I don't know if it makes sense to copy and paste what's existing rather than making conscious choices to only port what's actually useful.

@cee-chen
Copy link
Member

cee-chen commented Feb 5, 2024

Moving this back to a draft as it's still in progress / no longer in the review pipeline

@cee-chen cee-chen marked this pull request as draft February 5, 2024 20:17
@tkajtoch tkajtoch closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants